[PATCHv2,net-next] dropwatch: Support monitoring of dropped frames

From: Izabela Bakollari <izabela.bakollari@gmail.com>

Dropwatch is a utility that monitors dropped frames by having userspace
record them over the dropwatch protocol over a file. This augument
allows live monitoring of dropped frames using tools like tcpdump.

With this feature, dropwatch allows two additional commands (start and
stop interface) which allows the assignment of a net_device to the
dropwatch protocol. When assinged, dropwatch will clone dropped frames,
and receive them on the assigned interface, allowing tools like tcpdump
to monitor for them.

With this feature, create a dummy ethernet interface (ip link add dev
dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
these new commands, and then monitor dropped frames in real time by
running tcpdump -i dummy0.

Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
---
Changes in v2:
- protect the dummy ethernet interface from being changed by another
thread/cpu
---
 include/uapi/linux/net_dropmon.h |  3 ++
 net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

Thank you for your review. I am working on a patch v3 and will apply
your suggestions where possible.

Best,
Izabela

On Mon, Aug 31, 2020 at 3:18 PM Michal Schmidt <mschmidt@redhat.com> wrote:
>
> Dne 04. 08. 20 v 18:09 izabela.bakollari@gmail.com napsala:
> > From: Izabela Bakollari <izabela.bakollari@gmail.com>
> >
> > Dropwatch is a utility that monitors dropped frames by having userspace
> > record them over the dropwatch protocol over a file. This augument
> > allows live monitoring of dropped frames using tools like tcpdump.
> >
> > With this feature, dropwatch allows two additional commands (start and
> > stop interface) which allows the assignment of a net_device to the
> > dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> > and receive them on the assigned interface, allowing tools like tcpdump
> > to monitor for them.
> >
> > With this feature, create a dummy ethernet interface (ip link add dev
> > dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> > these new commands, and then monitor dropped frames in real time by
> > running tcpdump -i dummy0.
> >
> > Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> > ---
> > Changes in v2:
> > - protect the dummy ethernet interface from being changed by another
> > thread/cpu
> > ---
> >   include/uapi/linux/net_dropmon.h |  3 ++
> >   net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
> >   2 files changed, 87 insertions(+)
> [...]
> > @@ -255,6 +259,21 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> >
> >   out:
> >       spin_unlock_irqrestore(&data->lock, flags);
> > +     spin_lock_irqsave(&interface_lock, flags);
> > +     if (interface && interface != skb->dev) {
> > +             skb = skb_clone(skb, GFP_ATOMIC);
>
> I suggest naming the cloned skb "nskb". Less potential for confusion
> that way.
>
> > +             if (skb) {
> > +                     skb->dev = interface;
> > +                     spin_unlock_irqrestore(&interface_lock, flags);
> > +                     netif_receive_skb(skb);
> > +             } else {
> > +                     spin_unlock_irqrestore(&interface_lock, flags);
> > +                     pr_err("dropwatch: Not enough memory to clone dropped skb\n");
>
> Maybe avoid logging the error here. In NET_DM_ALERT_MODE_PACKET mode,
> drop monitor does not log about the skb_clone() failure either.
> We don't want to open the possibility to flood the logs in case this
> somehow gets triggered by every packet.
>
> A coding style suggestion - can you rearrange it so that the error path
> code is spelled out first? Then the regular path does not have to be
> indented further:
>
>        nskb = skb_clone(skb, GFP_ATOMIC);
>        if (!nskb) {
>                spin_unlock_irqrestore(&interface_lock, flags);
>                return;
>        }
>
>        /* ... implicit else ... Proceed normally ... */
>
> > +                     return;
> > +             }
> > +     } else {
> > +             spin_unlock_irqrestore(&interface_lock, flags);
> > +     }
> >   }
> >
> >   static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> > @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
> >       return -EOPNOTSUPP;
> >   }
> >
> > +static int net_dm_interface_start(struct net *net, const char *ifname)
> > +{
> > +     struct net_device *nd = dev_get_by_name(net, ifname);
> > +
> > +     if (nd)
> > +             interface = nd;
> > +     else
> > +             return -ENODEV;
> > +
> > +     return 0;
>
> Similarly here, consider:
>
>    if (!nd)
>            return -ENODEV;
>
>    interface = nd;
>    return 0;
>
> But maybe I'm nitpicking ...
>
> > +}
> > +
> > +static int net_dm_interface_stop(struct net *net, const char *ifname)
> > +{
> > +     dev_put(interface);
> > +     interface = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +     struct net *net = sock_net(skb->sk);
> > +     char ifname[IFNAMSIZ];
> > +
> > +     if (net_dm_is_monitoring())
> > +             return -EBUSY;
> > +
> > +     memset(ifname, 0, IFNAMSIZ);
> > +     nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> > +
> > +     switch (info->genlhdr->cmd) {
> > +     case NET_DM_CMD_START_IFC:
> > +             if (!interface)
> > +                     return net_dm_interface_start(net, ifname);
> > +             else
> > +                     return -EBUSY;
> > +     case NET_DM_CMD_STOP_IFC:
> > +             if (interface)
> > +                     return net_dm_interface_stop(net, interface->name);
> > +             else
> > +                     return -ENODEV;
>
> ... and here too.
>
> Best regards,
> Michal
>