netdev - Re: [PATCH net-next] net: Avoid spurious rx_dropped increases with tap and rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 May 2020 17:25:21 +0900
From:   Benjamin Poirier <bpoirier@...ulusnetworks.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net-next] net: Avoid spurious rx_dropped increases with
 tap and rx_handler

On 2020-05-24 22:41 -0700, Eric Dumazet wrote:
> On Sun, May 24, 2020 at 10:02 PM Benjamin Poirier
> <bpoirier@...ulusnetworks.com> wrote:
> >
> > Consider an skb which doesn't match a ptype_base/ptype_specific handler. If
> > this skb is delivered to a ptype_all handler, it does not count as a drop.
> > However, if the skb is also processed by an rx_handler which returns
> > RX_HANDLER_PASS, the frame is now counted as a drop because pt_prev was
> > reset. An example of this situation is an LLDP frame received on a bridge
> > port while lldpd is listening on a packet socket with ETH_P_ALL (ex. by
> > specifying `lldpd -c`).
> >
> > Fix by adding an extra condition variable to record if the skb was
> > delivered to a packet tap before running an rx_handler.
> >
> > The situation is similar for RX_HANDLER_EXACT frames so their accounting is
> > also changed. OTOH, the behavior is unchanged for RX_HANDLER_ANOTHER frames
> > - they are accounted according to what happens with the new skb->dev.
> >
> > Fixes: caf586e5f23c ("net: add a core netdev->rx_dropped counter")
> 
> I disagree.
> 
> > Message-Id: <20200522011420.263574-1-bpoirier@...ulusnetworks.com>
[...]
> > +               if (!rx_tapped) {
> >  drop:
> > -               if (!deliver_exact)
> > -                       atomic_long_inc(&skb->dev->rx_dropped);
> > -               else
> > -                       atomic_long_inc(&skb->dev->rx_nohandler);
> > +                       if (!deliver_exact)
> > +                               atomic_long_inc(&skb->dev->rx_dropped);
> > +                       else
> > +                               atomic_long_inc(&skb->dev->rx_nohandler);
> > +               }
> 
> This does not make sense to me.
> 
> Here we call kfree_skb() meaning this packet is _dropped_.
> I understand it does not please some people, because they do not
> always understand the meaning of this counter, but it is a mere fact.

IMO, the core of the issue is calling deliver_skb() before running the
rx_handler function. The rx_handler may not even attempt to deliver the
skb anywhere (RX_HANDLER_PASS). Because of that deliver_skb() call, the
packet_type handler (packet_rcv()) makes a spurious skb_clone(). Once a
useless copy has been made, it has to be freed somewhere. That's why
with my patch there may be kfree_skb() without an increase of the
dropped counter.

> 
> Fact that a packet capture made a clone of this packet should not
> matter, tcpdump should not hide that a packet is _dropped_.

What this patch intends to fix is that the behavior is inconsistent
depending on whether the interface has an rx_handler or not:
	eth0 nomaster -> tapped frames don't count as dropped
	eth0 master br0 -> tapped frames count as dropped
That has been the case since the counter was introduced.

This patch makes tapped frames uniformly not count as drops. If we
should move in the other direction (always count frames that were only
delivered to ptype_all handlers as dropped), I'll work on a different
patch.