| Message ID | 20250110092641.85905-7-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | af_unix: Set skb drop reason in every kfree_skb() path. | expand |
On Fri, Jan 10, 2025 at 06:26:35PM +0900, Kuniyuki Iwashima wrote: > This is a follow-up of commit d460b04bc452 ("af_unix: Clean up > error paths in unix_stream_sendmsg()."). > > If we initialise skb with NULL in unix_stream_sendmsg(), we can > reuse the existing out_pipe label for the SEND_SHUTDOWN check. > > Let's rename do it and adjust the existing label as out_pipe_lock. > > While at it, size and data_len are moved to the while loop scope. > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/unix/af_unix.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index b190ea8b8e9d..6505eeab9957 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c ... > @@ -2285,16 +2283,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, > } > } > > - if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) { > - if (!(msg->msg_flags & MSG_NOSIGNAL)) > - send_sig(SIGPIPE, current, 0); > - > - err = -EPIPE; > - goto out_err; > - } > + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) Hi Iwashima-san, I think you need to set reason here. Flagged by W=1 builds with clang-19. > + goto out_pipe; > > while (sent < len) { > - size = len - sent; > + int size = len - sent; > + int data_len; > > if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES)) { > skb = sock_alloc_send_pskb(sk, 0, 0, ...
From: Simon Horman <horms@kernel.org> Date: Fri, 10 Jan 2025 11:43:44 +0000 > On Fri, Jan 10, 2025 at 06:26:35PM +0900, Kuniyuki Iwashima wrote: > > This is a follow-up of commit d460b04bc452 ("af_unix: Clean up > > error paths in unix_stream_sendmsg()."). > > > > If we initialise skb with NULL in unix_stream_sendmsg(), we can > > reuse the existing out_pipe label for the SEND_SHUTDOWN check. > > > > Let's rename do it and adjust the existing label as out_pipe_lock. > > > > While at it, size and data_len are moved to the while loop scope. > > > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > net/unix/af_unix.c | 23 +++++++++-------------- > > 1 file changed, 9 insertions(+), 14 deletions(-) > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > index b190ea8b8e9d..6505eeab9957 100644 > > --- a/net/unix/af_unix.c > > +++ b/net/unix/af_unix.c > > ... > > > @@ -2285,16 +2283,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, > > } > > } > > > > - if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) { > > - if (!(msg->msg_flags & MSG_NOSIGNAL)) > > - send_sig(SIGPIPE, current, 0); > > - > > - err = -EPIPE; > > - goto out_err; > > - } > > + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) > > Hi Iwashima-san, > > I think you need to set reason here. > > Flagged by W=1 builds with clang-19. Hi Simon, I didn't set it here because skb == NULL and kfree_skb() doesn't touch reason, and KMSAN won't complain about uninit. Should I use SKB_NOT_DROPPED_YET or drop patch 6 or leave it as is ? What do you think ? Thanks!
On Sat, Jan 11, 2025 at 12:22:31AM +0900, Kuniyuki Iwashima wrote: > From: Simon Horman <horms@kernel.org> > Date: Fri, 10 Jan 2025 11:43:44 +0000 > > On Fri, Jan 10, 2025 at 06:26:35PM +0900, Kuniyuki Iwashima wrote: > > > This is a follow-up of commit d460b04bc452 ("af_unix: Clean up > > > error paths in unix_stream_sendmsg()."). > > > > > > If we initialise skb with NULL in unix_stream_sendmsg(), we can > > > reuse the existing out_pipe label for the SEND_SHUTDOWN check. > > > > > > Let's rename do it and adjust the existing label as out_pipe_lock. > > > > > > While at it, size and data_len are moved to the while loop scope. > > > > > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > > --- > > > net/unix/af_unix.c | 23 +++++++++-------------- > > > 1 file changed, 9 insertions(+), 14 deletions(-) > > > > > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > > > index b190ea8b8e9d..6505eeab9957 100644 > > > --- a/net/unix/af_unix.c > > > +++ b/net/unix/af_unix.c > > > > ... > > > > > @@ -2285,16 +2283,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, > > > } > > > } > > > > > > - if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) { > > > - if (!(msg->msg_flags & MSG_NOSIGNAL)) > > > - send_sig(SIGPIPE, current, 0); > > > - > > > - err = -EPIPE; > > > - goto out_err; > > > - } > > > + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) > > > > Hi Iwashima-san, > > > > I think you need to set reason here. > > > > Flagged by W=1 builds with clang-19. > > Hi Simon, > > I didn't set it here because skb == NULL and kfree_skb() > doesn't touch reason, and KMSAN won't complain about uninit. > > Should I use SKB_NOT_DROPPED_YET or drop patch 6 or leave > it as is ? > > What do you think ? My vote is that SKB_NOT_DROPPED_YET is not appropriate here. Maybe SKB_DROP_REASON_SOCKET_CLOSE since it is in SEND_SHUTDOWN state?
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index b190ea8b8e9d..6505eeab9957 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2250,13 +2250,11 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) { struct sock *sk = sock->sk; + struct sk_buff *skb = NULL; struct sock *other = NULL; - int err, size; - struct sk_buff *skb; - int sent = 0; struct scm_cookie scm; bool fds_sent = false; - int data_len; + int err, sent = 0; err = scm_send(sock, msg, &scm, false); if (err < 0) @@ -2285,16 +2283,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, } } - if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) { - if (!(msg->msg_flags & MSG_NOSIGNAL)) - send_sig(SIGPIPE, current, 0); - - err = -EPIPE; - goto out_err; - } + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) + goto out_pipe; while (sent < len) { - size = len - sent; + int size = len - sent; + int data_len; if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES)) { skb = sock_alloc_send_pskb(sk, 0, 0, @@ -2347,7 +2341,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, if (sock_flag(other, SOCK_DEAD) || (other->sk_shutdown & RCV_SHUTDOWN)) - goto out_pipe; + goto out_pipe_unlock; maybe_add_creds(skb, sock, other); scm_stat_add(other, skb); @@ -2370,8 +2364,9 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, return sent; -out_pipe: +out_pipe_unlock: unix_state_unlock(other); +out_pipe: if (!sent && !(msg->msg_flags & MSG_NOSIGNAL)) send_sig(SIGPIPE, current, 0); err = -EPIPE;
This is a follow-up of commit d460b04bc452 ("af_unix: Clean up error paths in unix_stream_sendmsg()."). If we initialise skb with NULL in unix_stream_sendmsg(), we can reuse the existing out_pipe label for the SEND_SHUTDOWN check. Let's rename do it and adjust the existing label as out_pipe_lock. While at it, size and data_len are moved to the while loop scope. Suggested-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- net/unix/af_unix.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)