linux-kernel - Re: [PATCH 01/19] Coccinelle: locks: identify callers of spin_lock{,_irq,

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:   Thu, 9 Mar 2017 15:37:24 -0600
From:   Julia Cartwright <julia@...com>
To:     Julia Lawall <julia.lawall@...6.fr>
CC:     Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <mmarek@...e.com>, <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        <cocci@...teme.lip6.fr>
Subject: Re: [PATCH 01/19] Coccinelle: locks: identify callers of
 spin_lock{,_irq,_irqsave}() in irqchip implementations

Hello Julia-

Thanks for the feedback.

On Thu, Mar 09, 2017 at 09:15:21PM +0100, Julia Lawall wrote:
> > +@...ch2 depends on match@
> > +identifier match.__irq_mask;
> > +identifier data;
> > +identifier x;
> > +identifier l;
> > +type T;
> > +position j0;
> > +expression flags;
> > +@@
> > +	static void __irq_mask(struct irq_data *data)
> > +	{
> > +		...
> > +		T *x;
> > +		...
> > +(
> > +		spin_lock_irqsave(&x->l@j0, flags);
> > +|
> > +		spin_lock_irq(&x->l@j0);
> > +|
> > +		spin_lock(&x->l@j0);
> > +)
> > +		...
> > +	}
> 
> I guess that here you want a match if there is a lock anywhere in the
> function?

Most generally, yes.  Any invocation of spin_lock{,_irq,_irqsave}() in
the irq_mask callback of an irq_chip implementation (irq_mask is only
_one_ such problematic callback, but a fairly representative one).

I should probably introduce a more generic report-mode rule which more
matches spin_lock{,_irq,_irqsave}(e), leaving this rule which requires
the lock accessed through local pointer-indirection only used as a
condition for patch mode.

I'll play with this a bit.

> Currently, the rule requires that the lock appear on every
> control-flow path.  If you put exists after depends on match in the rule
> header, it will match if there exists a control-flow patch that contains a
> local call.

Thanks, this makes sense.

> Also, ... matches the shortest path between the pattern before the ... and
> the pattern after.  Thus, x would have to be the first variable in the
> function of pointer type.  To eliminate this constraint, put when any on
> each of the ...s.  This will additionally allow more than one lock call in
> the function.
> 
> All in all, I would suggest the following for this rule:
> 
> @match2 depends on match exists@
> identifier match.__irq_mask;
> identifier data;
> identifier x;
> identifier l;
> type T;
> position j0;
> expression flags;
> @@
> 	static void __irq_mask(struct irq_data *data)
>         {
>                 ... when any
>                 T *x;
>                 ... when any
> (
>                 spin_lock_irqsave(&x->l@j0, flags);
> |
>                 spin_lock_irq(&x->l@j0);
> |
>                 spin_lock(&x->l@j0);
> )
>                 ... when any
>         }

Great, thanks, Julia!

   - The Other Julia

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)