Skip to content

Commit

Permalink
bpf: sockmap, fix introduced strparser recursive lock
Browse files Browse the repository at this point in the history
Originally there was a race where removing a psock from the sock map while
it was also receiving an skb and calling sk_psock_data_ready(). It was
possible the removal code would NULL/set the data_ready callback while
concurrently calling the hook from receive path. The fix was to wrap the
access in sk_callback_lock to ensure the saved_data_ready pointer didn't
change under us. There was some discussion around doing a larger change
to ensure we could use READ_ONCE/WRITE_ONCE over the callback, but that
was for *next kernels not stable fixes.

But, we unfortunately introduced a regression with the fix because there
is another path into this code (that didn't have a test case) through
the stream parser. The stream parser runs with the lower lock which means
we get the following splat and lock up.

 ============================================
 WARNING: possible recursive locking detected
 6.10.0-rc2 torvalds#59 Not tainted
 --------------------------------------------
 test_sockmap/342 is trying to acquire lock:
 ffff888007a87228 (clock-AF_INET){++--}-{2:2}, at:
 sk_psock_skb_ingress_enqueue (./include/linux/skmsg.h:467
 net/core/skmsg.c:555)

 but task is already holding lock:
 ffff888007a87228 (clock-AF_INET){++--}-{2:2}, at:
 sk_psock_strp_data_ready (net/core/skmsg.c:1120)

To fix ensure we do not grap lock when we reach this code through the
strparser.

Fixes: 6648e61 ("bpf, skmsg: Fix NULL pointer dereference in sk_psock_skb_ingress_enqueue")
Reported-by: Vincent Whitchurch <[email protected]>
Signed-off-by: John Fastabend <[email protected]>
  • Loading branch information
jrfastab authored and intel-lab-lkp committed Jun 25, 2024
1 parent c73a968 commit f416e85
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 3 deletions.
9 changes: 7 additions & 2 deletions include/linux/skmsg.h
Original file line number Diff line number Diff line change
Expand Up @@ -461,13 +461,18 @@ static inline void sk_psock_put(struct sock *sk, struct sk_psock *psock)
sk_psock_drop(sk, psock);
}

static inline void sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
static inline void __sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
{
read_lock_bh(&sk->sk_callback_lock);
if (psock->saved_data_ready)
psock->saved_data_ready(sk);
else
sk->sk_data_ready(sk);
}

static inline void sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
{
read_lock_bh(&sk->sk_callback_lock);
__sk_psock_data_ready(sk, psock);
read_unlock_bh(&sk->sk_callback_lock);
}

Expand Down
5 changes: 4 additions & 1 deletion net/core/skmsg.c
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,10 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
msg->skb = skb;

sk_psock_queue_msg(psock, msg);
sk_psock_data_ready(sk, psock);
if (skb_bpf_strparser(skb))
__sk_psock_data_ready(sk, psock);
else
sk_psock_data_ready(sk, psock);
return copied;
}

Expand Down

0 comments on commit f416e85

Please sign in to comment.