Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Paolo Abeni <pabeni@redhat.com>]

On 10/25/24 11:32, Matthieu Baerts (NGI0) wrote:
> @@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>  		bkup = 1;
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
> -			       __lookup_addr(pernet, &addr.addr);
> +	rcu_read_lock();
> +	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet, addr.addr.id) :
> +			       __lookup_addr_rcu(pernet, &addr.addr);
>  	if (!entry) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		return -EINVAL;
>  	}
>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "invalid addr flags");
>  		return -EINVAL;
>  	}
> @@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	changed = (addr.flags ^ entry->flags) & mask;
>  	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
>  	addr = *entry;
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
>  	return 0;
> 

I think we must retain the lock in this function, otherwise we could
end-up with an unexpected flag combination.

i.e. set_flag(MPTCP_PM_ADDR_FLAG_FULLMESH) and
set_flag(MPTCP_PM_ADDR_FLAG_SIGNAL) run concurrently on CPU1 and CPU2.
entry->flags could end-up having a single bit set instead of both, if
both CPUs read the entry before the other would store the new value.

I don't think _ONCE() annotations will be enough to avoid such thing
without a lock.

Cheers,

Paolo