Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback_lock

[Jakub Sitnicki <jakub@cloudflare.com>]

On Sat, Nov 19, 2022 at 07:08 PM +09, Tetsuo Handa wrote:
> On 2022/11/19 2:50, Jakub Sitnicki wrote:
>> Thanks for the patch, Tetsuo.
>> 
>> As Eric has pointed out [1], there is another problem - in addition to
>> sleeping in atomic context, I have also failed to use the write_lock
>> variant which disabled BH locally.
>> 
>> The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
>> because we grab sk_callback_lock in softirq context, which can then
>> block waiting on us if:
>
> Below is another approach I was thinking of, for reusing existing locks is prone
> to locking bugs like [2] and [3].
>
> I couldn't interpret "Write-protected by @sk_callback_lock." part because
> it does not say what lock is needed for protecting sk_user_data for read access.

sk_user_data is RCU-protected on reader-side. But we still need to
synchronize writers.

> Is it possible to use a mutex dedicated for l2tp_tunnel_destruct() (and optionally
> setup_udp_tunnel_sock_no_enable() in order not to create l2tp_tunnel_register_mutex =>
> cpu_hotplug_lock chain) ?

No, we need to a common lock to synchronize with other users in the net
stack (reuseport groups, sockmap/psock to name a couple).

> By the way I haven't heard an response on
>
>   Since userspace-supplied file descriptor has to be a datagram socket,
>   can we somehow copy the source/destination addresses from
>   userspace-supplied socket to kernel-created socket?
>
> at https://lkml.kernel.org/r/c9695548-3f27-dda1-3124-ec21da106741@I-love.SAKURA.ne.jp
> (that is, always create a new socket in order to be able to assign lockdep class
> before that socket is used).

This is a drive by fix for me to l2tp, so I might not be the best person
to ask, but I will take a look at the thread.

[...]