linux-kernel - Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Apr 2023 01:29:36 -0500
From:   Crystal Wood <oss@...error.net>
To:     Sean Anderson <sean.anderson@...o.com>,
        Li Yang <leoyang.li@....com>, linuxppc-dev@...ts.ozlabs.org,
        linux-arm-kernel@...ts.infradead.org
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Camelia Groza <camelia.groza@....com>,
        linux-kernel@...r.kernel.org, Roy Pledge <roy.pledge@....com>,
        "David S . Miller" <davem@...emloft.net>,
        Madalin Bucur <madalin.bucur@....com>
Subject: Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock

On Tue, 2023-04-11 at 11:09 -0400, Sean Anderson wrote:
> Hi Crystal,
> 
> On 4/4/23 12:04, Sean Anderson wrote:
> > On 4/4/23 11:33, Crystal Wood wrote:
> > > On Tue, 2023-04-04 at 10:55 -0400, Sean Anderson wrote:
> > > 
> > > > @@ -1456,11 +1456,11 @@ static void tqm_congestion_task(struct
> > > > work_struct
> > > > *work)
> > > >         union qm_mc_result *mcr;
> > > >         struct qman_cgr *cgr;
> > > >  
> > > > -       spin_lock_irq(&p->cgr_lock);
> > > > +       raw_spin_lock_irq(&p->cgr_lock);
> > > >         qm_mc_start(&p->p);
> > > >         qm_mc_commit(&p->p, QM_MCC_VERB_QUERYCONGESTION);
> > > >         if (!qm_mc_result_timeout(&p->p, &mcr)) {
> > > > -               spin_unlock_irq(&p->cgr_lock);
> > > > +               raw_spin_unlock_irq(&p->cgr_lock);
> > > 
> > > qm_mc_result_timeout() spins with a timeout of 10 ms which is very
> > > inappropriate for a raw lock.  What is the actual expected upper bound?
> > 
> > Hm, maybe we can move this qm_mc stuff outside cgr_lock? In most other
> > places they're called without cgr_lock, which implies that its usage
> > here is meant to synchronize against some other function.
> 
> Do you have any suggestions here? I think this should really be handled
> in a follow-up patch. If you think this code is waiting too long in a raw
> spinlock, the existing code can wait just as long with IRQs disabled.
> This patch doesn't change existing system responsiveness.

Well, AFAICT it expands the situations in which it happens from configuration
codepaths to stuff like congestion handling.  The proper fix would probably be
to use some mechanism other than smp_call_function_single() to run code on the
target cpu so that it can run with irqs enabled (or get confirmation that the
actual worst case is short enough), but barring that I guess at least
acknowledge the situation in a comment?

-Crystal