linux-kernel - Re: [PATCH] sg: atomize check and set sdp->exclude in sg

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: <20130605154106.GA2737@logfs.org>
Date:	Wed, 5 Jun 2013 11:41:07 -0400
From:	Jörn Engel <joern@...fs.org>
To:	vaughan <vaughan.cao@...cle.com>
Cc:	dgilbert@...erlog.com, JBottomley@...allels.com,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

On Thu, 6 June 2013 00:16:45 +0800, vaughan wrote:
> 于 2013年06月05日 21:27, Jörn Engel 写道:
> >On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
> >>
> >>Check and set sdp->exclude should be atomic when set in sg_open().
> >
> >The patch is line-wrapped.  More importantly, it doesn't seem to do
> It's shorter than the original line, so I just leave it like this...

Sure.  What I meant by line-wrapped is that your mailer mangled the
patch.  Those two lines should have been one:
> >>-                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
> >>? 0 : set_exclude(sdp, 1)));

> >what your description indicates it should do.  And lastly, does this
> >fix a bug, possibly even one you have a testcase for, or was it found
> >by code inspection?
> I found it by code inspection. A race condition may happen with the
> old code if two threads are both trying to open the same sg with
> O_EXCL simultaneously. It's possible that they both find fsds list
> is empty and get_exclude(sdp) returns 0, then they both call
> set_exclude() and break out from wait_event_interruptible and resume
> open. So it's necessary to check again with sg_open_exclusive_lock
> held to ensure only one can set sdp->exclude and return >0 to break
> out from wait_event loop.

Makes sense.  And reading the code again, I have to wonder what monkey
came up with the get_exclude/set_exclude functions.

Can I sucker you into a slightly larger cleanup?  I think the entire
"get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)" should be simplified.
And once you add the try_set_exclude(), set_exclude will only ever do
clear_exclude, so you might as well rename and simplify that as well.

Let no good deed go unpunished.

Jörn

--
It's just what we asked for, but not what we want!
-- anonymous
--
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/