Re: [PATCH bpf v3 1/4] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

[John Fastabend <john.fastabend@gmail.com>]

Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@rbox.co>
> Date: Sun,  7 Jul 2024 23:28:22 +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>
> 

Why does af_unix put the oob data on the sk_receive_queue()? Wouldn't it
be enough to just have the ousk->oob_skb hold the reference to the skb?

I think for TCP/UDP at least I'll want to handle MSG_OOB data correctly.
For redirect its probably fine to just drop or skip it, but when we are
just reading recv msgs and parsing/observing it would be nice to not change
how the application works. In practice I don't recall anyone reporting
issues on TCP side though from incorrectly handling URG data.

From TCP side I believe we can fix the OOB case by checking the oob queue
before doing the recvmsg handling. If the urg data wasn't on the general
sk_receive_queue we could do similar here for af_unix? My argument for
URG not working for redirect would be to let userspace handle it if they
cared.

Thanks.

> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> 
> Thanks!
> 
> 
> > ---
> >  net/unix/af_unix.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
> >  net/unix/unix_bpf.c |  3 +++
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index 142f56770b77..11cb5badafb6 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -2667,10 +2667,49 @@ 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);
> > +	if (!skb)
> > +		return err;
> > +
> > +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> > +	if (unlikely(skb == READ_ONCE(u->oob_skb))) {
> > +		bool drop = false;
> > +
> > +		unix_state_lock(sk);
> > +
> > +		if (sock_flag(sk, SOCK_DEAD)) {
> > +			unix_state_unlock(sk);
> > +			kfree_skb(skb);
> > +			return -ECONNRESET;
> > +		}
> > +
> > +		spin_lock(&sk->sk_receive_queue.lock);
> > +		if (likely(skb == u->oob_skb)) {
> > +			WRITE_ONCE(u->oob_skb, NULL);
> > +			drop = true;
> > +		}
> > +		spin_unlock(&sk->sk_receive_queue.lock);
> > +
> > +		unix_state_unlock(sk);
> > +
> > +		if (drop) {
> > +			WARN_ON_ONCE(skb_unref(skb));
> > +			kfree_skb(skb);
> > +			return -EAGAIN;
> > +		}
> > +	}
> > +#endif
> > +
> > +	return recv_actor(sk, skb);
> >  }
> >  
> >  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.2