| Message ID | 06ac4e517bff69c23646594d3b404b9ffb51001c.1676491484.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: add a refcnt in sctp_stream_priorities to avoid a nested loop | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 46 this patch: 46 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1 this patch: 1 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 46 this patch: 46 |
| netdev/checkpatch | warning | WARNING: line length of 96 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, Feb 15, 2023 at 03:04:44PM -0500, Xin Long wrote: > With this refcnt added in sctp_stream_priorities, we don't need to > traverse all streams to check if the prio is used by other streams > when freeing one stream's prio in sctp_sched_prio_free_sid(). This > can avoid a nested loop (up to 65535 * 65535), which may cause a > stuck as Ying reported: > > watchdog: BUG: soft lockup - CPU#23 stuck for 26s! [ksoftirqd/23:136] > Call Trace: > <TASK> > sctp_sched_prio_free_sid+0xab/0x100 [sctp] > sctp_stream_free_ext+0x64/0xa0 [sctp] > sctp_stream_free+0x31/0x50 [sctp] > sctp_association_free+0xa5/0x200 [sctp] > > Note that it doesn't need to use refcount_t type for this counter, > as its accessing is always protected under the sock lock. > > Fixes: 9ed7bfc79542 ("sctp: fix memory leak in sctp_stream_outq_migrate()") > Reported-by: Ying Xu <yinxu@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Hello, On Wed, 2023-02-15 at 15:04 -0500, Xin Long wrote: > With this refcnt added in sctp_stream_priorities, we don't need to > traverse all streams to check if the prio is used by other streams > when freeing one stream's prio in sctp_sched_prio_free_sid(). This > can avoid a nested loop (up to 65535 * 65535), which may cause a > stuck as Ying reported: > > watchdog: BUG: soft lockup - CPU#23 stuck for 26s! [ksoftirqd/23:136] > Call Trace: > <TASK> > sctp_sched_prio_free_sid+0xab/0x100 [sctp] > sctp_stream_free_ext+0x64/0xa0 [sctp] > sctp_stream_free+0x31/0x50 [sctp] > sctp_association_free+0xa5/0x200 [sctp] > > Note that it doesn't need to use refcount_t type for this counter, > as its accessing is always protected under the sock lock. > > Fixes: 9ed7bfc79542 ("sctp: fix memory leak in sctp_stream_outq_migrate()") > Reported-by: Ying Xu <yinxu@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > include/net/sctp/structs.h | 1 + > net/sctp/stream_sched_prio.c | 47 +++++++++++++----------------------- > 2 files changed, 18 insertions(+), 30 deletions(-) > > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > index afa3781e3ca2..e1f6e7fc2b11 100644 > --- a/include/net/sctp/structs.h > +++ b/include/net/sctp/structs.h > @@ -1412,6 +1412,7 @@ struct sctp_stream_priorities { > /* The next stream in line */ > struct sctp_stream_out_ext *next; > __u16 prio; > + __u16 users; I'm sorry for the late feedback. Reading the commit message, it looks like this counter could reach at least 64K. Is a __u16 integer wide enough? Thanks! Paolo
On Mon, Feb 20, 2023 at 2:31 AM Paolo Abeni <pabeni@redhat.com> wrote: > > Hello, > > On Wed, 2023-02-15 at 15:04 -0500, Xin Long wrote: > > With this refcnt added in sctp_stream_priorities, we don't need to > > traverse all streams to check if the prio is used by other streams > > when freeing one stream's prio in sctp_sched_prio_free_sid(). This > > can avoid a nested loop (up to 65535 * 65535), which may cause a > > stuck as Ying reported: > > > > watchdog: BUG: soft lockup - CPU#23 stuck for 26s! [ksoftirqd/23:136] > > Call Trace: > > <TASK> > > sctp_sched_prio_free_sid+0xab/0x100 [sctp] > > sctp_stream_free_ext+0x64/0xa0 [sctp] > > sctp_stream_free+0x31/0x50 [sctp] > > sctp_association_free+0xa5/0x200 [sctp] > > > > Note that it doesn't need to use refcount_t type for this counter, > > as its accessing is always protected under the sock lock. > > > > Fixes: 9ed7bfc79542 ("sctp: fix memory leak in sctp_stream_outq_migrate()") > > Reported-by: Ying Xu <yinxu@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > include/net/sctp/structs.h | 1 + > > net/sctp/stream_sched_prio.c | 47 +++++++++++++----------------------- > > 2 files changed, 18 insertions(+), 30 deletions(-) > > > > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > > index afa3781e3ca2..e1f6e7fc2b11 100644 > > --- a/include/net/sctp/structs.h > > +++ b/include/net/sctp/structs.h > > @@ -1412,6 +1412,7 @@ struct sctp_stream_priorities { > > /* The next stream in line */ > > struct sctp_stream_out_ext *next; > > __u16 prio; > > + __u16 users; > > I'm sorry for the late feedback. Reading the commit message, it looks > like this counter could reach at least 64K. Is a __u16 integer wide > enough? Normally, it will be enough. The max count of out streams is SCTP_MAX_STREAM (65535), and each stream will hold the "prio_head" once only. So when there's only one "prio_head", and 65535 streams will hold this "prio_head" 65535 times, this is at most. However, I notice in sctp_sched_prio_set() when changing one stream's "prio_head", it picks and holds the new one, then releases the old one. If the new one is the same as the old one while all streams are already holding it, it may reach 65536 in a moment. Although it will go back to 65535 soon, it's better to add a check to avoid this: @@ -168,6 +170,9 @@ static int sctp_sched_prio_set(struct sctp_stream *stream, __u16 sid, struct sctp_stream_priorities *prio_head, *old; bool reschedule = false; + if (soute->prio_head && soute->prio_head->prio == prio) + return 0; + I will post v2. Thanks.
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index afa3781e3ca2..e1f6e7fc2b11 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1412,6 +1412,7 @@ struct sctp_stream_priorities { /* The next stream in line */ struct sctp_stream_out_ext *next; __u16 prio; + __u16 users; }; struct sctp_stream_out_ext { diff --git a/net/sctp/stream_sched_prio.c b/net/sctp/stream_sched_prio.c index 42d4800f263d..66404a101b25 100644 --- a/net/sctp/stream_sched_prio.c +++ b/net/sctp/stream_sched_prio.c @@ -25,6 +25,18 @@ static void sctp_sched_prio_unsched_all(struct sctp_stream *stream); +static struct sctp_stream_priorities *sctp_sched_prio_head_get(struct sctp_stream_priorities *p) +{ + p->users++; + return p; +} + +static void sctp_sched_prio_head_put(struct sctp_stream_priorities *p) +{ + if (p && --p->users == 0) + kfree(p); +} + static struct sctp_stream_priorities *sctp_sched_prio_new_head( struct sctp_stream *stream, int prio, gfp_t gfp) { @@ -38,6 +50,7 @@ static struct sctp_stream_priorities *sctp_sched_prio_new_head( INIT_LIST_HEAD(&p->active); p->next = NULL; p->prio = prio; + p->users = 1; return p; } @@ -53,7 +66,7 @@ static struct sctp_stream_priorities *sctp_sched_prio_get_head( */ list_for_each_entry(p, &stream->prio_list, prio_sched) { if (p->prio == prio) - return p; + return sctp_sched_prio_head_get(p); if (p->prio > prio) break; } @@ -70,7 +83,7 @@ static struct sctp_stream_priorities *sctp_sched_prio_get_head( */ break; if (p->prio == prio) - return p; + return sctp_sched_prio_head_get(p); } /* If not even there, allocate a new one. */ @@ -154,7 +167,6 @@ static int sctp_sched_prio_set(struct sctp_stream *stream, __u16 sid, struct sctp_stream_out_ext *soute = sout->ext; struct sctp_stream_priorities *prio_head, *old; bool reschedule = false; - int i; prio_head = sctp_sched_prio_get_head(stream, prio, gfp); if (!prio_head) @@ -166,20 +178,7 @@ static int sctp_sched_prio_set(struct sctp_stream *stream, __u16 sid, if (reschedule) sctp_sched_prio_sched(stream, soute); - if (!old) - /* Happens when we set the priority for the first time */ - return 0; - - for (i = 0; i < stream->outcnt; i++) { - soute = SCTP_SO(stream, i)->ext; - if (soute && soute->prio_head == old) - /* It's still in use, nothing else to do here. */ - return 0; - } - - /* No hits, we are good to free it. */ - kfree(old); - + sctp_sched_prio_head_put(old); return 0; } @@ -206,20 +205,8 @@ static int sctp_sched_prio_init_sid(struct sctp_stream *stream, __u16 sid, static void sctp_sched_prio_free_sid(struct sctp_stream *stream, __u16 sid) { - struct sctp_stream_priorities *prio = SCTP_SO(stream, sid)->ext->prio_head; - int i; - - if (!prio) - return; - + sctp_sched_prio_head_put(SCTP_SO(stream, sid)->ext->prio_head); SCTP_SO(stream, sid)->ext->prio_head = NULL; - for (i = 0; i < stream->outcnt; i++) { - if (SCTP_SO(stream, i)->ext && - SCTP_SO(stream, i)->ext->prio_head == prio) - return; - } - - kfree(prio); } static void sctp_sched_prio_enqueue(struct sctp_outq *q,
With this refcnt added in sctp_stream_priorities, we don't need to traverse all streams to check if the prio is used by other streams when freeing one stream's prio in sctp_sched_prio_free_sid(). This can avoid a nested loop (up to 65535 * 65535), which may cause a stuck as Ying reported: watchdog: BUG: soft lockup - CPU#23 stuck for 26s! [ksoftirqd/23:136] Call Trace: <TASK> sctp_sched_prio_free_sid+0xab/0x100 [sctp] sctp_stream_free_ext+0x64/0xa0 [sctp] sctp_stream_free+0x31/0x50 [sctp] sctp_association_free+0xa5/0x200 [sctp] Note that it doesn't need to use refcount_t type for this counter, as its accessing is always protected under the sock lock. Fixes: 9ed7bfc79542 ("sctp: fix memory leak in sctp_stream_outq_migrate()") Reported-by: Ying Xu <yinxu@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/sctp/structs.h | 1 + net/sctp/stream_sched_prio.c | 47 +++++++++++++----------------------- 2 files changed, 18 insertions(+), 30 deletions(-)