[PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[David Decotigny]

From: David Decotigny <ddecotig@google.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@google.com>


---
 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 952a54763358..5ad7ac674daa 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))
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[Jakub Kicinski]

On Mon, 12 Dec 2022 23:38:01 -0800 David Decotigny wrote:
> From: David Decotigny <ddecotig@google.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.

Hm, how does this square with the spirit of 859bd2ef1fc11 ?

Would be great to hear from David, IDK if he's around or off until 
next year.

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[Mahesh Bandewar (महेश बंडेवार)]

On Wed, Dec 14, 2022 at 8:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 12 Dec 2022 23:38:01 -0800 David Decotigny wrote:
> > From: David Decotigny <ddecotig@google.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.
>
> Hm, how does this square with the spirit of 859bd2ef1fc11 ?
>
Devid's (Decotigny) patch is adding the distinction between admin-down
from carrier-off which could be transient. So in my belief, it's
correcting / enhancing the behavior that David (Ahern) introduced but
we can hear from David himself :)

> Would be great to hear from David, IDK if he's around or off until
> next year.

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[Jakub Kicinski]

On Wed, 14 Dec 2022 22:18:04 -0800 David Decotigny wrote:
> I don't think this patch is changing that part of the behavior: we still
> flush the cached nd entries when the link flaps. What we don't remove are
> the pneigh_entry-es (ip neigh add proxy ...) attached to the device where
> the link flaps: those are configured once and this patch ensures that they
> survive the link flaps as long as the netdev stays admin-up. When
> the netdev is brought admin-down, we keep the behavior we had before the
> patch.

Makes sense. This is not urgent, tho, right?

David A, do you agree and should we treat this as a fix with

Fixes: 859bd2ef1fc1 ("net: Evict neighbor entries on carrier down")

added?

Reminder: please bottom post on the list

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[David Decotigny]

(answer below)

On Thu, Dec 15, 2022 at 11:05 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 14 Dec 2022 22:18:04 -0800 David Decotigny wrote:
> > I don't think this patch is changing that part of the behavior: we still
> > flush the cached nd entries when the link flaps. What we don't remove are
> > the pneigh_entry-es (ip neigh add proxy ...) attached to the device where
> > the link flaps: those are configured once and this patch ensures that they
> > survive the link flaps as long as the netdev stays admin-up. When
> > the netdev is brought admin-down, we keep the behavior we had before the
> > patch.
>
> Makes sense. This is not urgent, tho, right?

Not that kind of urgent.

FTR, in the v2 you suggested to use NUD_PERMANENT, I can try to see
how this would look like. Note that this will make the patch larger
and more intrusive, and with potentially a behavior change for whoever
uses the netlink API directly instead of the iproute2 implementation
for ip neigh X proxy things.

>
> David A, do you agree and should we treat this as a fix with
>
> Fixes: 859bd2ef1fc1 ("net: Evict neighbor entries on carrier down")

Thanks.

>
> added?
>
> Reminder: please bottom post on the list

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[Jakub Kicinski]

On Thu, 15 Dec 2022 12:36:32 -0800 David Decotigny wrote:
> > Makes sense. This is not urgent, tho, right?  
> 
> Not that kind of urgent.
> 
> FTR, in the v2 you suggested to use NUD_PERMANENT,

I think that was Alex. I don't have a strong preference. I could see
arguments being made in both directions (basically whether it's more
important to leave objects which are clearly not cache vs we care 
more about consistent behavior based on the permanent flag itself).

Let's limit the reposts until experts are in town ;)

>  I can try to see how this would look like. Note that this will make
> the patch larger and more intrusive, and with potentially a behavior
> change for whoever uses the netlink API directly instead of the
> iproute2 implementation for ip neigh X proxy things.

Re: [PATCH v1 1/1] net: neigh: persist proxy config across link flaps

[David Ahern]

On 12/15/22 2:16 PM, Jakub Kicinski wrote:
> On Thu, 15 Dec 2022 12:36:32 -0800 David Decotigny wrote:
>>> Makes sense. This is not urgent, tho, right?  
>>
>> Not that kind of urgent.
>>
>> FTR, in the v2 you suggested to use NUD_PERMANENT,
> 
> I think that was Alex. I don't have a strong preference. I could see
> arguments being made in both directions (basically whether it's more
> important to leave objects which are clearly not cache vs we care 
> more about consistent behavior based on the permanent flag itself).
> 
> Let's limit the reposts until experts are in town ;)
> 
>>  I can try to see how this would look like. Note that this will make
>> the patch larger and more intrusive, and with potentially a behavior
>> change for whoever uses the netlink API directly instead of the
>> iproute2 implementation for ip neigh X proxy things.

My thinking is inline with Alex's comment on v2.