* [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct()
@ 2024-11-19 15:22 Alexandra Winter
2024-11-19 18:01 ` David Wei
2024-11-26 9:10 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Alexandra Winter @ 2024-11-19 15:22 UTC (permalink / raw)
To: David Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
Cc: netdev, linux-s390, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Thorsten Winkler, Simon Horman, Sidraya Jayagond
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);
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)
--
2.45.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct()
2024-11-19 15:22 [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct() Alexandra Winter
@ 2024-11-19 18:01 ` David Wei
2024-11-26 9:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: David Wei @ 2024-11-19 18:01 UTC (permalink / raw)
To: Alexandra Winter, David Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet
Cc: netdev, linux-s390, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Thorsten Winkler, Simon Horman, Sidraya Jayagond
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct()
2024-11-19 15:22 [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct() Alexandra Winter
2024-11-19 18:01 ` David Wei
@ 2024-11-26 9:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-11-26 9:10 UTC (permalink / raw)
To: Alexandra Winter
Cc: davem, kuba, pabeni, edumazet, netdev, linux-s390, hca, gor,
agordeev, borntraeger, svens, twinkler, horms, sidraya
Hello:
This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Tue, 19 Nov 2024 16:22:19 +0100 you 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.
>
> [...]
Here is the summary with links:
- [net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct()
https://git.kernel.org/netdev/net/c/ebaf81317e42
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-26 9:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-19 15:22 [PATCH net] s390/iucv: MSG_PEEK causes memory leak in iucv_sock_destruct() Alexandra Winter
2024-11-19 18:01 ` David Wei
2024-11-26 9:10 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox