| Message ID | 20241119152219.3712168-1-wintera@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct() | expand |
On 2024-11-19 07:22, Alexandra Winter wrote: > From: Sidraya Jayagond <sidraya@linux.ibm.com> > > Passing MSG_PEEK flag to skb_recv_datagram() increments skb refcount > (skb->users) and iucv_sock_recvmsg() does not decrement skb refcount > at exit. > This results in skb memory leak in skb_queue_purge() and WARN_ON in > iucv_sock_destruct() during socket close. To fix this decrease > skb refcount by one if MSG_PEEK is set in order to prevent memory > leak and WARN_ON. > > WARNING: CPU: 2 PID: 6292 at net/iucv/af_iucv.c:286 iucv_sock_destruct+0x144/0x1a0 [af_iucv] > CPU: 2 PID: 6292 Comm: afiucv_test_msg Kdump: loaded Tainted: G W 6.10.0-rc7 #1 > Hardware name: IBM 3931 A01 704 (z/VM 7.3.0) > Call Trace: > [<001587c682c4aa98>] iucv_sock_destruct+0x148/0x1a0 [af_iucv] > [<001587c682c4a9d0>] iucv_sock_destruct+0x80/0x1a0 [af_iucv] > [<001587c704117a32>] __sk_destruct+0x52/0x550 > [<001587c704104a54>] __sock_release+0xa4/0x230 > [<001587c704104c0c>] sock_close+0x2c/0x40 > [<001587c702c5f5a8>] __fput+0x2e8/0x970 > [<001587c7024148c4>] task_work_run+0x1c4/0x2c0 > [<001587c7023b0716>] do_exit+0x996/0x1050 > [<001587c7023b13aa>] do_group_exit+0x13a/0x360 > [<001587c7023b1626>] __s390x_sys_exit_group+0x56/0x60 > [<001587c7022bccca>] do_syscall+0x27a/0x380 > [<001587c7049a6a0c>] __do_syscall+0x9c/0x160 > [<001587c7049ce8a8>] system_call+0x70/0x98 > Last Breaking-Event-Address: > [<001587c682c4a9d4>] iucv_sock_destruct+0x84/0x1a0 [af_iucv] > > Fixes: eac3731bd04c ("[S390]: Add AF_IUCV socket support") > Reviewed-by: Alexandra Winter <wintera@linux.ibm.com> > Reviewed-by: Thorsten Winkler <twinkler@linux.ibm.com> > Signed-off-by: Sidraya Jayagond <sidraya@linux.ibm.com> > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > --- > The following mailaddresses are no longer active: > Frank Pavlic <fpavlic@de.ibm.com> (blamed_fixes:1/1=100%) > Martin Schwidefsky <schwidefsky@de.ibm.com> (blamed_fixes:1/1=100%) > --- > net/iucv/af_iucv.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c > index c00323fa9eb6..7929df08d4e0 100644 > --- a/net/iucv/af_iucv.c > +++ b/net/iucv/af_iucv.c > @@ -1236,7 +1236,9 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, > return -EOPNOTSUPP; > > /* receive/dequeue next skb: > - * the function understands MSG_PEEK and, thus, does not dequeue skb */ > + * the function understands MSG_PEEK and, thus, does not dequeue skb > + * only refcount is increased. > + */ > skb = skb_recv_datagram(sk, flags, &err); I checked the call graph and `flags` is passed through: skb_recv_datagram() -> __skb_recv_datagram() -> __skb_try_recv_datagram() -> __skb_try_recv_from_queue() If MSG_PEEK is set and a valid skb is returned then skb->users is incremented. > if (!skb) { > if (sk->sk_shutdown & RCV_SHUTDOWN) > @@ -1252,9 +1254,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, > > cskb = skb; > if (skb_copy_datagram_msg(cskb, offset, msg, copied)) { > - if (!(flags & MSG_PEEK)) > - skb_queue_head(&sk->sk_receive_queue, skb); > - return -EFAULT; > + err = -EFAULT; > + goto err_out; Previous behaviour is unchanged. Now if MSG_PEEK is set then skb->users is decremented. At this point skb is guaranteed to be valid. > } > > /* SOCK_SEQPACKET: set MSG_TRUNC if recv buf size is too small */ > @@ -1271,11 +1272,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, > err = put_cmsg(msg, SOL_IUCV, SCM_IUCV_TRGCLS, > sizeof(IUCV_SKB_CB(skb)->class), > (void *)&IUCV_SKB_CB(skb)->class); > - if (err) { > - if (!(flags & MSG_PEEK)) > - skb_queue_head(&sk->sk_receive_queue, skb); > - return err; > - } > + if (err) > + goto err_out; Same as above. > > /* Mark read part of skb as used */ > if (!(flags & MSG_PEEK)) { > @@ -1331,8 +1329,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, > /* SOCK_SEQPACKET: return real length if MSG_TRUNC is set */ > if (sk->sk_type == SOCK_SEQPACKET && (flags & MSG_TRUNC)) > copied = rlen; > + if (flags & MSG_PEEK) > + skb_unref(skb); I checked that all return paths with MSG_PEEK and a valid skb result in skb_unref(). The remaining return paths either have !MSG_PEEK or !skb: (1) if (!skb) { if (sk->sk_shutdown & RCV_SHUTDOWN) return 0; return err; } (2) if (!(flags & MSG_PEEK)) { ... if (iucv->transport == AF_IUCV_TRANS_HIPER) { atomic_inc(&iucv->msg_recv); if (atomic_read(&iucv->msg_recv) > iucv->msglimit) { WARN_ON(1); iucv_sock_close(sk); return -EFAULT; } } ... } > > return copied; > + > +err_out: > + if (!(flags & MSG_PEEK)) > + skb_queue_head(&sk->sk_receive_queue, skb); > + else > + skb_unref(skb); > + > + return err; > } > > static inline __poll_t iucv_accept_poll(struct sock *parent) Reviewed-by: David Wei <dw@davidwei.uk>
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c index c00323fa9eb6..7929df08d4e0 100644 --- a/net/iucv/af_iucv.c +++ b/net/iucv/af_iucv.c @@ -1236,7 +1236,9 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, return -EOPNOTSUPP; /* receive/dequeue next skb: - * the function understands MSG_PEEK and, thus, does not dequeue skb */ + * the function understands MSG_PEEK and, thus, does not dequeue skb + * only refcount is increased. + */ skb = skb_recv_datagram(sk, flags, &err); if (!skb) { if (sk->sk_shutdown & RCV_SHUTDOWN) @@ -1252,9 +1254,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, cskb = skb; if (skb_copy_datagram_msg(cskb, offset, msg, copied)) { - if (!(flags & MSG_PEEK)) - skb_queue_head(&sk->sk_receive_queue, skb); - return -EFAULT; + err = -EFAULT; + goto err_out; } /* SOCK_SEQPACKET: set MSG_TRUNC if recv buf size is too small */ @@ -1271,11 +1272,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, err = put_cmsg(msg, SOL_IUCV, SCM_IUCV_TRGCLS, sizeof(IUCV_SKB_CB(skb)->class), (void *)&IUCV_SKB_CB(skb)->class); - if (err) { - if (!(flags & MSG_PEEK)) - skb_queue_head(&sk->sk_receive_queue, skb); - return err; - } + if (err) + goto err_out; /* Mark read part of skb as used */ if (!(flags & MSG_PEEK)) { @@ -1331,8 +1329,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg, /* SOCK_SEQPACKET: return real length if MSG_TRUNC is set */ if (sk->sk_type == SOCK_SEQPACKET && (flags & MSG_TRUNC)) copied = rlen; + if (flags & MSG_PEEK) + skb_unref(skb); return copied; + +err_out: + if (!(flags & MSG_PEEK)) + skb_queue_head(&sk->sk_receive_queue, skb); + else + skb_unref(skb); + + return err; } static inline __poll_t iucv_accept_poll(struct sock *parent)