linux-kernel - Re: [PATCH 1/1 repost #1] DRM: don't enable irqs in locking

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, 29 Jul 2008 00:31:13 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Thomas Hellström <thomas@...gstengraphics.com>
Cc:	Dave Airlie <airlied@...il.com>, Jiri Slaby <jirislaby@...il.com>,
	airlied@...ux.ie, dri-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1 repost #1] DRM: don't enable irqs in locking

On Mon, 28 Jul 2008 22:32:45 +0200 Thomas Hellstr__m <thomas@...gstengraphics.com> wrote:

> Dave Airlie wrote:
> > On Fri, Jul 25, 2008 at 6:42 PM, Jiri Slaby <jirislaby@...il.com> wrote:
> >   
> >> drm_lock_take(); and drm_lock_free(); are called from
> >> drm_locked_tasklet_func(); which disables interrupts when grabbing its
> >> spinlock.
> >>
> >> Don't allow these locking functions to re-enable interrupts when
> >> the tasklet expects them disabled. I.e. use spin_lock_irqsave instead of
> >> spin_lock_bh (with their unlock opposites).
> >>     
> >
> > Hmm this has bounced through 2-3 variations.. Thomas any ideas what
> > the final correct answer is?
> >
> > Dave.
> >   
> Hmm,
> Yes, this bug could occur, but the remedy is not to use 
> spin_lock_irqsave() for lock_data::spinlock but to avoid calling 
> drm_lock_take with the drm_device::tasklet_lock held with irqs disabled.
> I'll see if I can come up with a patch.
> 

The code in drivers/gpu/drm/drm_lock.c needs some serious help in the
kerneldoc department.

/**
 * Take the heavyweight lock.
 *
 * \param lock lock pointer.
 * \param context locking context.
 * \return one if the lock is held, or zero otherwise.
 *
 * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
 */

The /** leadin specifically introduces a kerneldoc-formatted comment. 
Yet that comment uses some strange home-made way of denoting function
arguments.

The comments could quite easily be converted to kerneldoc form, which
would be the best thing to do here.

While you're there, please reformat the comments (drm_idlelock_take(),
mainly) to fit in 80-cols.  The code carefully does this, but the block
comments then go and ruin it all.

--
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/