lkml.org 
[lkml]   [2022]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH net-next v2 1/1] net: neigh: persist proxy config across link flaps
From
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.

\
 
 \ /
  Last update: 2022-12-16 00:02    [W:0.106 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site