netdev - Re: [PATCH net-next 3/8] net: enqueue_to_backlog() change vs not running device

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]

Message-ID: <CAL+tcoDq5m5zK9G9t8v8zpRZEhp=BDwYft2jnC4pqwhS1RHPTA@mail.gmail.com>
Date: Fri, 29 Mar 2024 16:44:09 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 3/8] net: enqueue_to_backlog() change vs not
 running device

On Fri, Mar 29, 2024 at 2:31 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Mar 29, 2024 at 4:21 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > On Fri, Mar 29, 2024 at 1:07 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > If the device attached to the packet given to enqueue_to_backlog()
> > > is not running, we drop the packet.
> > >
> > > But we accidentally increase sd->dropped, giving false signals
> > > to admins: sd->dropped should be reserved to cpu backlog pressure,
> > > not to temporary glitches at device dismantles.
> >
> > It seems that drop action happening is intended in this case (see
> > commit e9e4dd3267d0c ("net: do not process device backlog during
> > unregistration")). We can see the strange/unexpected behaviour at
> > least through simply taking a look at /proc/net/softnet_stat file.
>
> I disagree.
>
> We are dismantling a device, temporary drops are expected, and this
> patch adds a more precise drop_reason.
>
> I have seen admins being worried about this counter being not zero on
> carefully tuned hosts.
>
> If you think we have to carry these drops forever in
> /proc/net/softnet_stat, you will have give
> a more precise reason than "This was intentionally added in 2015"
>
> e9e4dd3267d0c5234c changelog was very long, but said nothing
> about why sd->dropped _had_ to be updated.

Indeed, the author might think the skb is dropped in the RPS process
so it's necessary to record that and reflect these actions in this
proc file.

Don't get me wrong. I'm not against what you did, just proposing my
concern about changing the meaning/understanding of 'drop'.

Thanks,
Jason