[PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps

[PATCH net-next v2 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>


---
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))
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps

[Alexander H Duyck]

On Wed, 2022-12-14 at 15:20 -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.
> 
> Signed-off-by: David Decotigny <ddecotig@google.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?

Re: [PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps

[Alexander Duyck]

On Thu, Dec 15, 2022 at 9:29 AM David Decotigny <decot@google.com> wrote:
>
>
> (comments inline below)
>
>
> On Thu, Dec 15, 2022 at 8:24 AM Alexander H Duyck <alexander.duyck@gmail.com> wrote:
>>
>> On Wed, 2022-12-14 at 15:20 -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.
>> >
>> > Signed-off-by: David Decotigny <ddecotig@google.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.

Re: [PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps

[David Ahern]

On 12/15/22 1:08 PM, Alexander Duyck wrote:
> On Thu, Dec 15, 2022 at 9:29 AM David Decotigny <decot@google.com> wrote:
>>
>>
>> (comments inline below)
>>
>>
>> On Thu, Dec 15, 2022 at 8:24 AM Alexander H Duyck <alexander.duyck@gmail.com> wrote:
>>>
>>> On Wed, 2022-12-14 at 15:20 -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.
>>>>
>>>> Signed-off-by: David Decotigny <ddecotig@google.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.

exactly.