linux-kernel - Re: [PATCH v2 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: <20130705173953.GA15089@logfs.org>
Date:	Fri, 5 Jul 2013 13:39:53 -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,
	vaughan.cao@...il.com, xitao.cao@...il.com
Subject: Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive
 open

Sorry about replying so late.

On Mon, 17 June 2013 21:10:53 +0800, vaughan wrote:
> 
> Rewrite the last patch.
> Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
> 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.
> Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().
> 
...
> @@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
>  	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
>  	int sg_tablesize;	/* adapter's max scatter-gather table size */
>  	u32 index;		/* device index number */
> -	/* sfds is protected by sg_index_lock */
> +	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
>  	struct list_head sfds;
> +	int toopen;		/* number of who are ready to open sg */
                                            ^
I think the 'toopen' is a bad choice.  I'm having trouble wrapping my
head around the semantics of this variable, your description feels a
bit handwavy, the main noun is missing in the command above, I think I
found one more overflow bug,...

What you ended up doing is reimplement a rw_semaphone.  Why not use
one instead?  down_write() for exclusive access, down_read() for
non-exclusive, _trylock variants for nonblocking opens, etc.

Would this work?

Jörn

--
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello
--
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/