[syzbot] memory leak in sctp_sched_prio_set

[syzbot] memory leak in sctp_sched_prio_set

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    b2d229d4ddb1 Linux 5.18-rc3
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17e439b8f00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2197cd22d3971cc5
dashboard link: https://syzkaller.appspot.com/bug?extid=29c402e56c4760763cc0
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17daf0af700000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10ae4d5cf00000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+29c402e56c4760763cc0@syzkaller.appspotmail.com

executing program
executing program
BUG: memory leak
unreferenced object 0xffff88810b472a00 (size 64):
  comm "syz-executor206", pid 3601, jiffies 4294944661 (age 12.290s)
  hex dump (first 32 bytes):
    00 2a 47 0b 81 88 ff ff 00 2a 47 0b 81 88 ff ff  .*G......*G.....
    10 2a 47 0b 81 88 ff ff 10 2a 47 0b 81 88 ff ff  .*G......*G.....
  backtrace:
    [<ffffffff83fa1634>] kmalloc include/linux/slab.h:581 [inline]
    [<ffffffff83fa1634>] sctp_sched_prio_new_head net/sctp/stream_sched_prio.c:33 [inline]
    [<ffffffff83fa1634>] sctp_sched_prio_get_head net/sctp/stream_sched_prio.c:77 [inline]
    [<ffffffff83fa1634>] sctp_sched_prio_set+0x2c4/0x370 net/sctp/stream_sched_prio.c:159
    [<ffffffff83f9b6a6>] sctp_stream_init_ext+0x86/0xf0 net/sctp/stream.c:176
    [<ffffffff83f86e0e>] sctp_sendmsg_to_asoc+0xc8e/0xdb0 net/sctp/socket.c:1807
    [<ffffffff83f8f77f>] sctp_sendmsg+0x99f/0x1030 net/sctp/socket.c:2027
    [<ffffffff83b7a315>] inet_sendmsg+0x45/0x70 net/ipv4/af_inet.c:819
    [<ffffffff837cb3e6>] sock_sendmsg_nosec net/socket.c:705 [inline]
    [<ffffffff837cb3e6>] sock_sendmsg+0x56/0x80 net/socket.c:725
    [<ffffffff837ce38c>] __sys_sendto+0x15c/0x200 net/socket.c:2040
    [<ffffffff837ce456>] __do_sys_sendto net/socket.c:2052 [inline]
    [<ffffffff837ce456>] __se_sys_sendto net/socket.c:2048 [inline]
    [<ffffffff837ce456>] __x64_sys_sendto+0x26/0x30 net/socket.c:2048
    [<ffffffff8451da45>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
    [<ffffffff8451da45>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
    [<ffffffff84600068>] entry_SYSCALL_64_after_hwframe+0x44/0xae



---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[PATCH] sctp: relese sctp_stream_priorities at sctp_stream_outq_migrate()

[Tetsuo Handa]

syzbot is reporting memory leak on sctp_stream_priorities [1], for
sctp_stream_outq_migrate() is resetting SCTP_SO(stream, i)->ext to NULL
without clearing SCTP_SO(new, i)->ext->prio_head list allocated by
sctp_sched_prio_new_head(). Since sctp_sched_prio_free() is too late to
clear if stream->outcnt was already shrunk or SCTP_SO(stream, i)->ext
was already NULL, add a callback for clearing that list before shrinking
stream->outcnt and/or resetting SCTP_SO(stream, i)->ext.

Link: https://syzkaller.appspot.com/bug?exrid=29c402e56c4760763cc0 [1]
Reported-by: syzbot <syzbot+29c402e56c4760763cc0@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
I can observe that the reproducer no longer reports memory leak. But
is this change correct and sufficient? Are there similar locations?

 include/net/sctp/stream_sched.h |  2 ++
 net/sctp/stream.c               |  3 +++
 net/sctp/stream_sched_prio.c    | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/net/sctp/stream_sched.h b/include/net/sctp/stream_sched.h
index 01a70b27e026..1a59d0f8ad79 100644
--- a/include/net/sctp/stream_sched.h
+++ b/include/net/sctp/stream_sched.h
@@ -28,6 +28,8 @@ struct sctp_sched_ops {
 	int (*init_sid)(struct sctp_stream *stream, __u16 sid, gfp_t gfp);
 	/* Frees the entire thing */
 	void (*free)(struct sctp_stream *stream);
+	/* Free one sid */
+	void (*free_sid)(struct sctp_stream *stream, __u16 sid);
 
 	/* Enqueue a chunk */
 	void (*enqueue)(struct sctp_outq *q, struct sctp_datamsg *msg);
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index ef9fceadef8d..845a8173181e 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -60,6 +60,7 @@ static void sctp_stream_outq_migrate(struct sctp_stream *stream,
 				     struct sctp_stream *new, __u16 outcnt)
 {
 	int i;
+	struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
 
 	if (stream->outcnt > outcnt)
 		sctp_stream_shrink_out(stream, outcnt);
@@ -77,6 +78,8 @@ static void sctp_stream_outq_migrate(struct sctp_stream *stream,
 	}
 
 	for (i = outcnt; i < stream->outcnt; i++) {
+		if (sched->free_sid)
+			sched->free_sid(stream, i);
 		kfree(SCTP_SO(stream, i)->ext);
 		SCTP_SO(stream, i)->ext = NULL;
 	}
diff --git a/net/sctp/stream_sched_prio.c b/net/sctp/stream_sched_prio.c
index 80b5a2c4cbc7..bde5537984a9 100644
--- a/net/sctp/stream_sched_prio.c
+++ b/net/sctp/stream_sched_prio.c
@@ -230,6 +230,25 @@ static void sctp_sched_prio_free(struct sctp_stream *stream)
 	}
 }
 
+static void sctp_sched_prio_free_sid(struct sctp_stream *stream, __u16 sid)
+{
+	struct sctp_stream_priorities *prio, *n;
+	struct sctp_stream_out *sout = SCTP_SO(stream, sid);
+	struct sctp_stream_out_ext *soute = sout->ext;
+	LIST_HEAD(list);
+
+	if (!soute)
+		return;
+	prio = soute->prio_head;
+	if (!prio || !list_empty(&prio->prio_sched))
+		return;
+	list_add(&prio->prio_sched, &list);
+	list_for_each_entry_safe(prio, n, &list, prio_sched) {
+		list_del_init(&prio->prio_sched);
+		kfree(prio);
+	}
+}
+
 static void sctp_sched_prio_enqueue(struct sctp_outq *q,
 				    struct sctp_datamsg *msg)
 {
@@ -323,6 +342,7 @@ static struct sctp_sched_ops sctp_sched_prio = {
 	.get = sctp_sched_prio_get,
 	.init = sctp_sched_prio_init,
 	.init_sid = sctp_sched_prio_init_sid,
+	.free_sid = sctp_sched_prio_free_sid,
 	.free = sctp_sched_prio_free,
 	.enqueue = sctp_sched_prio_enqueue,
 	.dequeue = sctp_sched_prio_dequeue,
-- 
2.34.1

Re: [PATCH] sctp: relese sctp_stream_priorities at sctp_stream_outq_migrate()

[Marcelo Ricardo Leitner]

On Wed, Nov 23, 2022 at 07:36:00PM +0900, Tetsuo Handa wrote:
> syzbot is reporting memory leak on sctp_stream_priorities [1], for
> sctp_stream_outq_migrate() is resetting SCTP_SO(stream, i)->ext to NULL
> without clearing SCTP_SO(new, i)->ext->prio_head list allocated by
> sctp_sched_prio_new_head(). Since sctp_sched_prio_free() is too late to
> clear if stream->outcnt was already shrunk or SCTP_SO(stream, i)->ext
> was already NULL, add a callback for clearing that list before shrinking
> stream->outcnt and/or resetting SCTP_SO(stream, i)->ext.
> 
> Link: https://syzkaller.appspot.com/bug?exrid=29c402e56c4760763cc0 [1]
> Reported-by: syzbot <syzbot+29c402e56c4760763cc0@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> I can observe that the reproducer no longer reports memory leak. But
> is this change correct and sufficient? Are there similar locations?

Thanks, but please see my email from yesterday. This is on the right
way but a cleanup then is possible:
https://lore.kernel.org/linux-sctp/Y31ct%2FlSXNTm9ev9@t14s.localdomain/

  Marcelo

Re: [PATCH] sctp: relese sctp_stream_priorities at sctp_stream_outq_migrate()

[Tetsuo Handa]

On 2022/11/23 22:57, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 23, 2022 at 07:36:00PM +0900, Tetsuo Handa wrote:
>> syzbot is reporting memory leak on sctp_stream_priorities [1], for
>> sctp_stream_outq_migrate() is resetting SCTP_SO(stream, i)->ext to NULL
>> without clearing SCTP_SO(new, i)->ext->prio_head list allocated by
>> sctp_sched_prio_new_head(). Since sctp_sched_prio_free() is too late to
>> clear if stream->outcnt was already shrunk or SCTP_SO(stream, i)->ext
>> was already NULL, add a callback for clearing that list before shrinking
>> stream->outcnt and/or resetting SCTP_SO(stream, i)->ext.
>>
>> Link: https://syzkaller.appspot.com/bug?exrid=29c402e56c4760763cc0 [1]
>> Reported-by: syzbot <syzbot+29c402e56c4760763cc0@syzkaller.appspotmail.com>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> ---
>> I can observe that the reproducer no longer reports memory leak. But
>> is this change correct and sufficient? Are there similar locations?
> 
> Thanks, but please see my email from yesterday. This is on the right
> way but a cleanup then is possible:
> https://lore.kernel.org/linux-sctp/Y31ct%2FlSXNTm9ev9@t14s.localdomain/

Oops, duplicated work again. Googling with this address did not hit, and
a thread at syzkaller-bugs group did not have your patch.

Please consider including syzbot+XXXXXXXXXXXXXXXXXXXX@syzkaller.appspotmail.com
and syzkaller-bugs@googlegroups.com into the Cc: list so that we can google for
your patch.