linux-kernel - Re: [PATCH] Video : Amba: Use in_interrupt() in clcdfb

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]

Message-ID: <20120311164243.GE13336@n2100.arm.linux.org.uk>
Date:	Sun, 11 Mar 2012 16:42:43 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	santosh prasad nayak <santoshprasadnayak@...il.com>
Cc:	FlorianSchandinat@....de, linux-fbdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] Video : Amba: Use in_interrupt() in clcdfb_sleep().

On Sun, Mar 11, 2012 at 10:07:18PM +0530, santosh prasad nayak wrote:
> On Sun, Mar 11, 2012 at 9:18 PM, Russell King - ARM Linux
> <linux@....linux.org.uk> wrote:
> > On Sun, Mar 11, 2012 at 08:49:27PM +0530, santosh prasad nayak wrote:
> >> On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
> >> <linux@....linux.org.uk> wrote:
> >> > in_interrupt() won't tell us if we're being called with spinlocks held,
> >> > which _is_ a possibility because this can be called from printk(), for
> >> > oops dumps and the like.
> >> >
> >> > in_interrupt() just means that we're inside a hard or soft interrupt,
> >> > or nmi.  It says nothing about whether msleep() is possible.
> >>
> >>
> >> in_atomic() is also not  error free.  I found following comment in
> >> include/linux/hardirq.h.  How do you handle it in non-preemptible
> >> kernel ?
> >>
> >> /*
> >>  * Are we running in atomic context?  WARNING: this macro cannot
> >>  * always detect atomic context; in particular, it cannot know about
> >>  * held spinlocks in non-preemptible kernels.  Thus it should not be
> >>  * used in the general case to determine whether sleeping is possible.
> >>  * Do not use in_atomic() in driver code.
> >>  */
> >> #define in_atomic()     ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
> >
> > That may be, but the fact of the matter is that no one has *ever*
> > reported an incident where this has failed at this point - and when
> > it does people will end up with a might_sleep() warning from msleep().
> >
> > Maybe those who are saying people should not use this should instead
> > be analysing why people use this, and suggest an alternative solution
> > to the problem instead of a basic and uninformative "you shouldn't use
> > this" statement.
> 
> The reason is given in the article.

At this point I'm just going to restate what I said above and below, so
I'm not even going to bother doing that, and instead just say that.  I'm
not arguing whether it's right or wrong.  I'm just stating that the only
solution I see is to get rid of msleep() in there entirely.

> http://lwn.net/Articles/274695/
> 
> "The in_atomic() macro works by checking whether preemption is
> disabled, which seems like the right thing to do. Handlers for events
> like hardware interrupts will disable preemption, but so will the
> acquisition of a spinlock. So this test appears to catch all of the
> cases where sleeping would be a bad idea. Certainly a number of people
> who have looked at this macro have come to that conclusion.
> 
> But if preemption has not been configured into the kernel in the first
> place, the kernel does not raise the "preemption count" when spinlocks
> are acquired. So, in this situation (which is common - many
> distributors still do not enable preemption in their kernels),
> in_atomic() has no way to know if the calling code holds any spinlocks
> or not. So it will return zero (indicating process context) even when
> spinlocks are held. And that could lead to kernel code thinking that
> it is running in process context (and acting accordingly) when, in
> fact, it is not."
> 
> 
> 
> 
> regards
> Santosh
> >
> > As I've said, if we aren't going to use this, then the only solution is
> > to completely omit the msleep() there and just say "sod you to running
> > anything else for 20ms while this driver busy-spins."  That's
> > ultimately the safe thing to do, and at the moment I see no other
> > alternative there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/