linux-kernel - Re: [PATCH v3 1/1] [SCSI] sg: fix race condition when do exclusive open

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: <20130715203730.GA21804@logfs.org>
Date:	Mon, 15 Jul 2013 16:37:30 -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 v3 1/1] [SCSI] sg: fix race condition when do exclusive
 open

On Mon, 8 July 2013 03:53:49 +0800, vaughan wrote:
> 
> Use rwsem to aid opens. Exclusive open has to get write lock and non-exclusive open should get read lock.
> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
> 
> Please pay attention to sdp->detached. Previously sg_open may also race with sg_remove. Now there are two points for sg_open to detect detached and finish itself. One is at sg_device lookup and the other is when trying to link new sfp into the sfds list in sg_add_sfp.
> I don't think it's necessary to do extra set_exclude and wake_up o_excl_wait in sg_release before, so I remove them and only do the cleanup in sg_remove_sfp.
> Although simple testcases are passed, I'm not certain if I've fixed it well, please give me any comments as you wish.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@...cle.com>
> ---
>  drivers/scsi/sg.c | 187 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 89 insertions(+), 98 deletions(-)

Had a quick read and I think I like the code.  What I still dislike is
that it merges several independent functional changes into one patch.

Can you create three patches, one for the rwsem part, one for pushing
the locking down to per-device locking and one for the rest?  That
would make it easier for me to understand and trust the individual
patches and for James/Linus to revert one, if it turns out to be bad.

Jörn

--
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare
--
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/