Re: [PATCH net-next 4/4] net, neigh: Add NTF_MANAGED flag for managed neighbor entries

[Daniel Borkmann <daniel@iogearbox.net>]

On 10/12/21 4:51 PM, David Ahern wrote:
> On 10/11/21 6:12 AM, Daniel Borkmann wrote:
>> @@ -66,12 +68,22 @@ enum {
>>   #define NUD_PERMANENT	0x80
>>   #define NUD_NONE	0x00
>>   
>> -/* NUD_NOARP & NUD_PERMANENT are pseudostates, they never change
>> - * and make no address resolution or NUD.
>> - * NUD_PERMANENT also cannot be deleted by garbage collectors.
>> +/* NUD_NOARP & NUD_PERMANENT are pseudostates, they never change and make no
>> + * address resolution or NUD.
>> + *
>> + * NUD_PERMANENT also cannot be deleted by garbage collectors. This holds true
>> + * for dynamic entries with NTF_EXT_LEARNED flag as well. However, upon carrier
>> + * down event, NUD_PERMANENT entries are not flushed whereas NTF_EXT_LEARNED
>> + * flagged entries explicitly are (which is also consistent with the routing
>> + * subsystem).
>> + *
>>    * When NTF_EXT_LEARNED is set for a bridge fdb entry the different cache entry
>>    * states don't make sense and thus are ignored. Such entries don't age and
>>    * can roam.
>> + *
>> + * NTF_EXT_MANAGED flagged neigbor entries are managed by the kernel on behalf
>> + * of a user space control plane, and automatically refreshed so that (if
>> + * possible) they remain in NUD_REACHABLE state.
> 
> switchdev use cases need this capability as well to offload routes.
> Similar functionality exists in mlxsw to resolve gateways. It would be
> good for this design to cover both needs - and that may be as simple as
> mlxsw setting the MANAGED flag on the entry to let the neigh subsystem
> takeover.

Ack, that would definitely be nice to reuse it there as well.

>>    */
>>   
>>   struct nda_cacheinfo {
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index 5245e888c981..eae73efa9245 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -122,6 +122,8 @@ static void neigh_mark_dead(struct neighbour *n)
>>   		list_del_init(&n->gc_list);
>>   		atomic_dec(&n->tbl->gc_entries);
>>   	}
>> +	if (!list_empty(&n->managed_list))
>> +		list_del_init(&n->managed_list);
>>   }
>>   
>>   static void neigh_update_gc_list(struct neighbour *n)
>> @@ -130,7 +132,6 @@ static void neigh_update_gc_list(struct neighbour *n)
>>   
>>   	write_lock_bh(&n->tbl->lock);
>>   	write_lock(&n->lock);
>> -
> 
> I like the extra newline - it makes locks stand out.

Ok, will drop, and add one to neigh_update_managed_list(), too.

>>   	if (n->dead)
>>   		goto out;
>>   
>> @@ -149,32 +150,59 @@ static void neigh_update_gc_list(struct neighbour *n)
>>   		list_add_tail(&n->gc_list, &n->tbl->gc_list);
>>   		atomic_inc(&n->tbl->gc_entries);
>>   	}
>> +out:
>> +	write_unlock(&n->lock);
>> +	write_unlock_bh(&n->tbl->lock);
>> +}
>> +
>> +static void neigh_update_managed_list(struct neighbour *n)
>> +{
>> +	bool on_managed_list, add_to_managed;
>> +
>> +	write_lock_bh(&n->tbl->lock);
>> +	write_lock(&n->lock);
>> +	if (n->dead)
>> +		goto out;
>> +
>> +	add_to_managed = n->flags & NTF_MANAGED;
>> +	on_managed_list = !list_empty(&n->managed_list);
>>   
>> +	if (!add_to_managed && on_managed_list)
>> +		list_del_init(&n->managed_list);
>> +	else if (add_to_managed && !on_managed_list)
>> +		list_add_tail(&n->managed_list, &n->tbl->managed_list);
>>   out:
>>   	write_unlock(&n->lock);
>>   	write_unlock_bh(&n->tbl->lock);
>>   }
>>   
>> -static bool neigh_update_ext_learned(struct neighbour *neigh, u32 flags,
>> -				     int *notify)
>> +static void neigh_update_flags(struct neighbour *neigh, u32 flags, int *notify,
>> +			       bool *gc_update, bool *managed_update)
>>   {
>> -	bool rc = false;
>> -	u32 ndm_flags;
>> +	u32 ndm_flags, old_flags = neigh->flags;
>>   
>>   	if (!(flags & NEIGH_UPDATE_F_ADMIN))
>> -		return rc;
>> +		return;
>> +
>> +	ndm_flags  = (flags & NEIGH_UPDATE_F_EXT_LEARNED) ? NTF_EXT_LEARNED : 0;
>> +	ndm_flags |= (flags & NEIGH_UPDATE_F_MANAGED) ? NTF_MANAGED : 0;
>>   
>> -	ndm_flags = (flags & NEIGH_UPDATE_F_EXT_LEARNED) ? NTF_EXT_LEARNED : 0;
>> -	if ((neigh->flags ^ ndm_flags) & NTF_EXT_LEARNED) {
>> +	if ((old_flags ^ ndm_flags) & NTF_EXT_LEARNED) {
>>   		if (ndm_flags & NTF_EXT_LEARNED)
>>   			neigh->flags |= NTF_EXT_LEARNED;
>>   		else
>>   			neigh->flags &= ~NTF_EXT_LEARNED;
>> -		rc = true;
>>   		*notify = 1;
>> +		*gc_update = true;
>> +	}
>> +	if ((old_flags ^ ndm_flags) & NTF_MANAGED) {
>> +		if (ndm_flags & NTF_MANAGED)
>> +			neigh->flags |= NTF_MANAGED;
>> +		else
>> +			neigh->flags &= ~NTF_MANAGED;
>> +		*notify = 1;
>> +		*managed_update = true;
>>   	}
>> -
>> -	return rc;
>>   }
>>   
>>   static bool neigh_del(struct neighbour *n, struct neighbour __rcu **np,
>> @@ -422,6 +450,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl,
>>   	refcount_set(&n->refcnt, 1);
>>   	n->dead		  = 1;
>>   	INIT_LIST_HEAD(&n->gc_list);
>> +	INIT_LIST_HEAD(&n->managed_list);
>>   
>>   	atomic_inc(&tbl->entries);
>>   out:
>> @@ -650,7 +679,8 @@ ___neigh_create(struct neigh_table *tbl, const void *pkey,
>>   	n->dead = 0;
>>   	if (!exempt_from_gc)
>>   		list_add_tail(&n->gc_list, &n->tbl->gc_list);
>> -
>> +	if (n->flags & NTF_MANAGED)
>> +		list_add_tail(&n->managed_list, &n->tbl->managed_list);
>>   	if (want_ref)
>>   		neigh_hold(n);
>>   	rcu_assign_pointer(n->next,
>> @@ -1205,8 +1235,6 @@ static void neigh_update_hhs(struct neighbour *neigh)
>>   	}
>>   }
>>   
>> -
>> -
>>   /* Generic update routine.
>>      -- lladdr is new lladdr or NULL, if it is not supplied.
>>      -- new    is new state.
>> @@ -1218,6 +1246,7 @@ static void neigh_update_hhs(struct neighbour *neigh)
>>   				if it is different.
>>   	NEIGH_UPDATE_F_ADMIN	means that the change is administrative.
>>   	NEIGH_UPDATE_F_USE	means that the entry is user triggered.
>> +	NEIGH_UPDATE_F_MANAGED	means that the entry will be auto-refreshed.
>>   	NEIGH_UPDATE_F_OVERRIDE_ISROUTER allows to override existing
>>   				NTF_ROUTER flag.
>>   	NEIGH_UPDATE_F_ISROUTER	indicates if the neighbour is known as
>> @@ -1225,17 +1254,15 @@ static void neigh_update_hhs(struct neighbour *neigh)
>>   
>>      Caller MUST hold reference count on the entry.
>>    */
>> -
>>   static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
>>   			  u8 new, u32 flags, u32 nlmsg_pid,
>>   			  struct netlink_ext_ack *extack)
>>   {
>> -	bool ext_learn_change = false;
>> -	u8 old;
>> -	int err;
>> -	int notify = 0;
>> -	struct net_device *dev;
>> +	bool gc_update = false, managed_update = false;
>>   	int update_isrouter = 0;
>> +	struct net_device *dev;
>> +	int err, notify = 0;
>> +	u8 old;
>>   
>>   	trace_neigh_update(neigh, lladdr, new, flags, nlmsg_pid);
>>   
>> @@ -1254,8 +1281,8 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
>>   	    (old & (NUD_NOARP | NUD_PERMANENT)))
>>   		goto out;
>>   
>> -	ext_learn_change = neigh_update_ext_learned(neigh, flags, &notify);
>> -	if (flags & NEIGH_UPDATE_F_USE) {
>> +	neigh_update_flags(neigh, flags, &notify, &gc_update, &managed_update);
>> +	if (flags & (NEIGH_UPDATE_F_USE | NEIGH_UPDATE_F_MANAGED)) {
>>   		new = old & ~NUD_PERMANENT;
> 
> so a neighbor entry can not be both managed and permanent, but you don't
> check for the combination in neigh_add and error out with a message to
> the user.

Good point, I'll error out if both NUD_PERMANENT and NTF_MANAGED is set in neigh_add().

Thanks for the review!
Daniel