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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 15 Mar 2019 10:13:49 -0400 From: Willem de Bruijn <willemdebruijn.kernel@...il.com> To: Maxime Chevallier <maxime.chevallier@...tlin.com> Cc: LKML <linux-kernel@...r.kernel.org>, Network Development <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>, Willem de Bruijn <willemb@...gle.com>, Eric Dumazet <edumazet@...gle.com>, Antoine Tenart <antoine.tenart@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com> Subject: Re: [RFC PATCH net] packets: Always register packet sk in the same order On Fri, Mar 15, 2019 at 5:03 AM Maxime Chevallier <maxime.chevallier@...tlin.com> wrote: > > Hi Willem, > > On Thu, 14 Mar 2019 11:24:54 -0400 > Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote: > > >On Thu, Mar 14, 2019 at 8:27 AM Maxime Chevallier > ><maxime.chevallier@...tlin.com> wrote: > >> > >> When using fanouts with AF_PACKET, the demux functions such as > >> fanout_demux_cpu will return an index in the fanout socket array, which > >> corresponds to the selected socket. > >> > >> The ordering of this array depends on the order the sockets were added > >> to a given fanout group, so for FANOUT_CPU this means sockets are bound > >> to cpus in the order they are configured, which is OK. > >> > >> However, when stopping then restarting the interface these sockets are > >> bound to, the sockets are reassigned to the fanout group in the reverse > >> order, due to the fact that they were inserted at the head of the > >> interface's AF_PACKET socket list. > >> > >> This means that traffic that was directed to the first socket in the > >> fanout group is now directed to the last one after an interface restart. > >> > >> In the case of FANOUT_CPU, traffic from CPU0 will be directed to the > >> socket that used to receive traffic from the last CPU after an interface > >> restart. > > > >The above assumes that sockets are added to a fanout group in the > >order in which they are created. That is a reasonable assumption, > >though not strictly required. This can be worked around in userspace > >by inserting in the inverse order. Still, good to fix. > > > >It does change the order of output of proc and diag, which is an > >unfortunately side effect. But insertion order is less surprising and > >I don't see a simple option to only traverse the sklist in inverse > >order on packet_notifier NETDEV_UP (which would avoid this). > > Thanks for the review ! Clearly the solution proposed by this patch has > side-effects, and as you said I don't see an easy way to do that in > another manner. > > >> This commit introduces a helper to add a socket at the tail of a list, > >> then uses it to register AF_PACKET sockets. > >> > >> Fixes: 808f5114a920 ("packet: convert socket list to RCU (v3)") > > > >Before this patch, insertion with sk_add_node is also at the head. > >This does not seem like the right commit? This behavior has probably > >existed as long as fanout. > > Yes I agree, I'm not even sure if this patch should be targeted to -net > or -net-next. If this is OK after reviews, I can resend it without RFC > and without this misleading Fixes tag. Sounds good. Since current behavior can split packets of the same flow across multiple sockets, even for fanout_demux_hash, I think this warrants net. Then the Fixes tag would be dc99f600698d ("packet: Add fanout support."). Please mention the reordering of proc and diag output in the commit message. Thanks
Powered by blists - more mailing lists