linux-kernel - Re: [PATCH 25/25] ipc: don't use [delayed_]work

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: <20121225163513.GF10220@mtj.dyndns.org>
Date:	Tue, 25 Dec 2012 08:35:13 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Borislav Petkov <bp@...en8.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

Hello, Borislav.

On Tue, Dec 25, 2012 at 11:46:14AM +0100, Borislav Petkov wrote:
> > It doesn't have anything to do with memory hotplug event itself.  It's
> > a generic memory access ordering / synchronization issue.  There just
> > isn't anything preventing test_bit() from seeing PENDING bit from
> > before clearing.
> 
> That's true. So is it that the whole PENDING bit testing was actually
> the wrong sync primitive to use in most cases and we actually really
> needed to disable interrupts around testing of that bit?

The IRQ disabling is for synchronization with other workqueue
operations.  Once PENDING is set, other workqueue operations may
busy-wait for the work item to be put on the appropriate queue so that
further operations can happen (e.g. flush / cancel).  So, if you have
to be atomic locally w.r.t. IRQs.

Memory ordering issue is the one which makes using test_bit() in
itself inadequate for gating the queue operation.  It could be all we
need is throwing in a smp_mb() before test_bit() to guarantee that
either clear PENDING is visible or all updates upto that point is
visible to the coming work item execution.  Not sure whether that
would be worthwhile tho.  The only case that may help is, I think, if
an already pending work item is queued very frequently from multiple
CPUs, which is unlikely and, if exists, indicates larger problems.

> Which would make your patches actually bugfixes.

Yes, why I'm talking about making [delayed_]work_pending() go away or
at least make it very ugly to use.

> /me goes and rereads 0/25 announcement mail.
> 
> Yes, it sounds like [delayed_]work_pending() needs to go or at least
> needs a big fat comment over it explaining that it is a special, cheaper
> alternative to be used *only* in conjunction with other synchronization.
> 
> Ok, thanks Tejun, for taking the time and explaining it again to me.

Thanks.

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