| Message ID | c5ba2194-dbb6-586d-992d-9dfcd27062e7@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sctp: relese sctp_stream_priorities at sctp_stream_outq_migrate() | expand |
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
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.
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,
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(+)