netdev - Re: [PATCH net] sctp: add a refcnt in sctp_stream

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 20 Feb 2023 11:28:50 -0500
From:   Xin Long <lucien.xin@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org,
        Eric Dumazet <edumazet@...gle.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        Zhengchao Shao <shaozhengchao@...wei.com>
Subject: Re: [PATCH net] sctp: add a refcnt in sctp_stream_priorities to avoid
 a nested loop

On Mon, Feb 20, 2023 at 2:31 AM Paolo Abeni <pabeni@...hat.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@...hat.com>
> > Signed-off-by: Xin Long <lucien.xin@...il.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.