Re: [PATCH] netlink: Fix the netlink socket malfunction due to concurrency

[Eric Dumazet <edumazet@google.com>]

On Fri, Aug 18, 2023 at 8:42 PM Qingjie Xing <xqjcool@gmail.com> wrote:
>
> The concurrent Invocation of netlink_attachskb() and netlink_recvmsg()
> on different CPUs causes malfunction of netlink socket.
>
> The concurrent scenario of netlink_recvmsg() and netlink_attachskb()
> as following:
>
> CPU A                           CPU B
> ========                        ========
> netlink_recvmsg()               netlink_attachskb()
>                                 [1]bit NETLINK_S_CONGESTED is set
>                                 netlink_overrun()
> netlink_rcv_wake()
> [2]sk_receive_queue is empty
> clear bit NETLINK_S_CONGESTED
>                                 [3]NETLINK_F_RECV_NO_ENOBUFS not set
>                                 set bit NETLINK_S_CONGESTED
>
> In this scenario, the socket's receive queue is empty. Additionally,
> due to the NETLINK_S_CONGESTED flag being set, all packets sent to
> this socket are discarded.
>
> To prevent this situation, we need to introduce a check for whether
> the socket receive buffer is full before setting the NETLINK_S_CONGESTED
> flag.
>
> Signed-off-by: Qingjie Xing <xqjcool@gmail.com>
> ---
>  net/netlink/af_netlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 383631873748..80bcce9acbfc 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -352,7 +352,8 @@ static void netlink_overrun(struct sock *sk)
>         struct netlink_sock *nlk = nlk_sk(sk);
>
>         if (!(nlk->flags & NETLINK_F_RECV_NO_ENOBUFS)) {
> -               if (!test_and_set_bit(NETLINK_S_CONGESTED,
> +               if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf
> +                       && !test_and_set_bit(NETLINK_S_CONGESTED,
>                                       &nlk_sk(sk)->state)) {
>                         sk->sk_err = ENOBUFS;
>                         sk_error_report(sk);

This does not look race-free to me.

sk->sk_rcvbuf can change anytime.

I wonder if we should instead remove all this CONGESTED thing and come
back to the fundamentals,
instead of having another bit mirroring some existing state.

Apparently part of it was added in

commit cd1df525da59c64244d27b4548ff5d132489488a
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Apr 17 06:47:05 2013 +0000

    netlink: add flow control for memory mapped I/O

    Add flow control for memory mapped RX. Since user-space usually doesn't
    invoke recvmsg() when using memory mapped I/O, flow control is performed
    in netlink_poll(). Dumps are allowed to continue if at least half of the
    ring frames are unused.

But I do not think we kept this memory mapped RX.