| Message ID | 20240620203009.2610301-1-mhal@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash | expand |
Sorry for not mentioning this before, but could you replace "net" with "bpf" in Subject and rebase the patch on bpf.git so that we can trigger the patchwork's CI ? https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git From: Michal Luczaj <mhal@rbox.co> Date: Thu, 20 Jun 2024 22:20:05 +0200 > AF_UNIX socket tracks the most recent OOB packet (in its receive queue) > with an `oob_skb` pointer. BPF redirecting does not account for that: when > an OOB packet is moved between sockets, `oob_skb` is left outdated. This > results in a single skb that may be accessed from two different sockets. > > Take the easy way out: silently drop MSG_OOB data targeting any socket that > is in a sockmap or a sockhash. Note that such silent drop is akin to the > fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS). > > For symmetry, forbid MSG_OOB in unix_bpf_recvmsg(). > > Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com> > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > Signed-off-by: Michal Luczaj <mhal@rbox.co> > --- > net/unix/af_unix.c | 30 +++++++++++++++++++++++++++++- > net/unix/unix_bpf.c | 3 +++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 5e695a9a609c..3a55d075f199 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -2653,10 +2653,38 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, > > static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor) > { > + struct unix_sock *u = unix_sk(sk); > + struct sk_buff *skb; > + int err; > + > if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED)) > return -ENOTCONN; > > - return unix_read_skb(sk, recv_actor); > + mutex_lock(&u->iolock); > + skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err); mutex_unlock(&u->iolock); I think we can drop mutex here as the skb is already unlinked and no receiver can touch it. and the below part can be like the following not to slow down the common case: if (!skb) return err; > + > +#if IS_ENABLED(CONFIG_AF_UNIX_OOB) > + if (skb) { if (unlikely(skb == READ_ONCE(u->oob_skb))) { > + bool drop = false; > + > + spin_lock(&sk->sk_receive_queue.lock); > + if (skb == u->oob_skb) { if (likely(skb == u->oob_skb)) { > + WRITE_ONCE(u->oob_skb, NULL); > + drop = true; > + } > + spin_unlock(&sk->sk_receive_queue.lock); > + > + if (drop) { > + WARN_ON_ONCE(skb_unref(skb)); > + kfree_skb(skb); > + skb = NULL; > + err = -EAGAIN; return -EAGAIN; > + } > + } > +#endif return recv_actor(sk, skb); Thanks! > + > + mutex_unlock(&u->iolock); > + return skb ? recv_actor(sk, skb) : err; > } > > static int unix_stream_read_generic(struct unix_stream_read_state *state, > diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c > index bd84785bf8d6..bca2d86ba97d 100644 > --- a/net/unix/unix_bpf.c > +++ b/net/unix/unix_bpf.c > @@ -54,6 +54,9 @@ static int unix_bpf_recvmsg(struct sock *sk, struct msghdr *msg, > struct sk_psock *psock; > int copied; > > + if (flags & MSG_OOB) > + return -EOPNOTSUPP; > + > if (!len) > return 0; > > -- > 2.45.1
On Thu, 20 Jun 2024 15:12:23 -0700 Kuniyuki Iwashima wrote: > Sorry for not mentioning this before, but could you replace "net" with > "bpf" in Subject and rebase the patch on bpf.git so that we can trigger > the patchwork's CI ? > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git netdev runs the BPF CI, too, FWIW. Open the patch in patchwork: https://patchwork.kernel.org/project/netdevbpf/patch/20240620203009.2610301-1-mhal@rbox.co/ Click on contest in "checks". Select Executor = "gh-bpf-ci". Click on "outputs", you should get to: https://github.com/kernel-patches/bpf/actions/runs/9607623089 If you click in context on the branch name it will take you to the tested branch: https://github.com/linux-netdev/testing/commits/net-next-2024-06-21--03-00 which had: af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash applied, 5th from the top.
On 6/21/24 00:12, Kuniyuki Iwashima wrote: > Sorry for not mentioning this before, but could you replace "net" with > "bpf" in Subject and rebase the patch on bpf.git so that we can trigger > the patchwork's CI ? No problem, will do. >> ... >> static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor) >> { >> + struct unix_sock *u = unix_sk(sk); >> + struct sk_buff *skb; >> + int err; >> + >> if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED)) >> return -ENOTCONN; >> >> - return unix_read_skb(sk, recv_actor); >> + mutex_lock(&u->iolock); >> + skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err); > > mutex_unlock(&u->iolock); > > I think we can drop mutex here as the skb is already unlinked > and no receiver can touch it. I guess you're right about the mutex. That said, double mea culpa, lack of state lock makes things racy: unix_stream_read_skb mutex_lock skb = skb_recv_datagram mutex_unlock spin_lock if (oob_skb == skb) { unix_release_sock if (u->oob_skb) { kfree_skb(u->oob_skb) u->oob_skb = NULL } oob_skb = NULL drop = true } spin_unlock if (drop) { skb_unref(skb) kfree_skb(skb) } In v2 I'll do what unix_stream_read_generic() does: take state lock and check for SOCK_DEAD. > and the below part can be like the following not to slow down > the common case: > > if (!skb) > return err; > >> + >> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB) >> + if (skb) { > > if (unlikely(skb == READ_ONCE(u->oob_skb))) { > > >> + bool drop = false; >> + >> + spin_lock(&sk->sk_receive_queue.lock); >> + if (skb == u->oob_skb) { > > if (likely(skb == u->oob_skb)) { > >> + WRITE_ONCE(u->oob_skb, NULL); >> + drop = true; >> + } >> + spin_unlock(&sk->sk_receive_queue.lock); >> + >> + if (drop) { >> + WARN_ON_ONCE(skb_unref(skb)); >> + kfree_skb(skb); >> + skb = NULL; >> + err = -EAGAIN; > return -EAGAIN; > >> + } >> + } >> +#endif > > return recv_actor(sk, skb); All right, thanks. So here's v2: https://lore.kernel.org/netdev/20240622223324.3337956-1-mhal@rbox.co/
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 5e695a9a609c..3a55d075f199 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2653,10 +2653,38 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor) { + struct unix_sock *u = unix_sk(sk); + struct sk_buff *skb; + int err; + if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED)) return -ENOTCONN; - return unix_read_skb(sk, recv_actor); + mutex_lock(&u->iolock); + skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err); + +#if IS_ENABLED(CONFIG_AF_UNIX_OOB) + if (skb) { + bool drop = false; + + spin_lock(&sk->sk_receive_queue.lock); + if (skb == u->oob_skb) { + WRITE_ONCE(u->oob_skb, NULL); + drop = true; + } + spin_unlock(&sk->sk_receive_queue.lock); + + if (drop) { + WARN_ON_ONCE(skb_unref(skb)); + kfree_skb(skb); + skb = NULL; + err = -EAGAIN; + } + } +#endif + + mutex_unlock(&u->iolock); + return skb ? recv_actor(sk, skb) : err; } static int unix_stream_read_generic(struct unix_stream_read_state *state, diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c index bd84785bf8d6..bca2d86ba97d 100644 --- a/net/unix/unix_bpf.c +++ b/net/unix/unix_bpf.c @@ -54,6 +54,9 @@ static int unix_bpf_recvmsg(struct sock *sk, struct msghdr *msg, struct sk_psock *psock; int copied; + if (flags & MSG_OOB) + return -EOPNOTSUPP; + if (!len) return 0;
AF_UNIX socket tracks the most recent OOB packet (in its receive queue) with an `oob_skb` pointer. BPF redirecting does not account for that: when an OOB packet is moved between sockets, `oob_skb` is left outdated. This results in a single skb that may be accessed from two different sockets. Take the easy way out: silently drop MSG_OOB data targeting any socket that is in a sockmap or a sockhash. Note that such silent drop is akin to the fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS). For symmetry, forbid MSG_OOB in unix_bpf_recvmsg(). Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com> Fixes: 314001f0bf92 ("af_unix: Add OOB support") Signed-off-by: Michal Luczaj <mhal@rbox.co> --- net/unix/af_unix.c | 30 +++++++++++++++++++++++++++++- net/unix/unix_bpf.c | 3 +++ 2 files changed, 32 insertions(+), 1 deletion(-)