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: Thu, 15 Dec 2022 12:08:34 -0800 From: Alexander Duyck <alexander.duyck@...il.com> To: David Decotigny <decot@...gle.com> Cc: David Decotigny <decot+git@...gle.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Nikolay Aleksandrov <razor@...ckwall.org>, David Ahern <dsahern@...nel.org>, "Denis V. Lunev" <den@...nvz.org>, Daniel Borkmann <daniel@...earbox.net>, Chen Zhongjin <chenzhongjin@...wei.com>, Yuwei Wang <wangyuweihx@...il.com>, Alexander Mikhalitsyn <alexander.mikhalitsyn@...tuozzo.com>, Thomas Zeitlhofer <thomas.zeitlhofer+lkml@...it.at> Subject: Re: [PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps On Thu, Dec 15, 2022 at 9:29 AM David Decotigny <decot@...gle.com> wrote: > > > (comments inline below) > > > On Thu, Dec 15, 2022 at 8:24 AM Alexander H Duyck <alexander.duyck@...il.com> wrote: >> >> On Wed, 2022-12-14 at 15:20 -0800, David Decotigny wrote: >> > From: David Decotigny <ddecotig@...gle.com> >> > >> > Without this patch, the 'ip neigh add proxy' config is lost when the >> > cable or peer disappear, ie. when the link goes down while staying >> > admin up. When the link comes back, the config is never recovered. >> > >> > This patch makes sure that such an nd proxy config survives a switch >> > or cable issue. >> > >> > Signed-off-by: David Decotigny <ddecotig@...gle.com> >> > >> > >> > --- >> > v1: initial revision >> > v2: same as v1, except rebased on top of latest net-next, and includes "net-next" in the description >> > >> > net/core/neighbour.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c >> > index f00a79fc301b..f4b65bbbdc32 100644 >> > --- a/net/core/neighbour.c >> > +++ b/net/core/neighbour.c >> > @@ -426,7 +426,10 @@ static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev, >> > { >> > write_lock_bh(&tbl->lock); >> > neigh_flush_dev(tbl, dev, skip_perm); >> > - pneigh_ifdown_and_unlock(tbl, dev); >> > + if (skip_perm) >> > + write_unlock_bh(&tbl->lock); >> > + else >> > + pneigh_ifdown_and_unlock(tbl, dev); >> > pneigh_queue_purge(&tbl->proxy_queue, dev ? dev_net(dev) : NULL, >> > tbl->family); >> > if (skb_queue_empty_lockless(&tbl->proxy_queue)) >> >> This seems like an agressive approach since it applies to all entries >> in the table, not just the permenant ones like occurs in >> neigh_flush_dev. >> >> I don't have much experience in this area of the code but it seems like >> you would specifically be wanting to keep only the permanant entries. >> Would it make sense ot look at rearranging pneigh_ifdown_and_unlock so >> that the code functioned more like neigh_flush_dev where it only >> skipped the permanant entries when skip_perm was set? >> > > The reason I am proposing this patch like it is is because these "proxy" entries appear to be a configuration attribute (similar to ip routes, coming from the sysadmin config), and not cached data (like ip neigh "normal" entries essentially coming from the outside). So I view them as fundamentally different kinds of objects [1], which they actually are in the code. And they are also updated from a vastly different context (sysadmin vs traffic). IMHO, it would seem natural that these proxy attributes (considered config attributes) would survive link flaps, whereas normal ip neigh cached entries without NUD_PERMANENT should not. And neither should survive admin down, the same way ip route does not survive admin down. This is what this patch proposes. > > Honoring NUD_PERMANENT (I assume that's what you are alluding to) would also work, and (with current iproute2 implementation [2]) would lead to the same result. But please consider the above. If really honoring NUD_PERMANENT is the required approach here, I am happy to revisit this patch. Please let me know. Yeah, I was referring to basically just limiting your changes to honor NUD_PERMANANT. Looking at pneigh_ifdown_and_unlock and comparing it to neigh_flush_dev it seems like it would make sense to just add the skip_perm argument there and then add the same logic at the start of the loop to eliminate the items you aren't going to flush/free. That way we aren't keeping around any more entries than those specifically that are supposed to be permanent.
Powered by blists - more mailing lists