linux-kernel - [PATCH] [SCSI] sg: late O

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-next>] [day] [month] [year] [list]

Message-ID: <52640025.60709@interlog.com>
Date:	Sun, 20 Oct 2013 12:09:09 -0400
From:	Douglas Gilbert <dgilbert@...erlog.com>
To:	SCSI development list <linux-scsi@...r.kernel.org>,
	vaughan <vaughan.cao@...cle.com>, Madper Xie <cxie@...hat.com>,
	James Bottomley <james.bottomley@...senpartnership.com>
CC:	linux-kernel <linux-kernel@...r.kernel.org>
Subject: [PATCH] [SCSI] sg: late O_EXCL fix for lk 3.12-rc

The lk 3.12.0-rc series contains a series of patches
from Vaughan Cao introduced by this post:
    [PATCH v7 0/4][SCSI] sg: fix race condition in sg_open
    http://marc.info/?l=linux-scsi&m=137774159020002&w=2

Doubt was thrown on the implementation by Madper Xie in
this thread:
    [Bug] 12.864681  BUG: lock held when returning to user space!
    http://marc.info/?l=linux-kernel&m=138121455020314&w=2

Well it is not a lock, it is a read-write semaphore down-ed
in sg_open() with the reasonable expectation that a
corresponding sg_release() will arrive thereupon that
read-write semaphore is upp-ed. I'm yet to find kernel
documentation that says that is illegal. There are even driver
examples on the 'net showing this same technique but that
doesn't prove it is correct.

Irrespective of 12.864681 while looking at this I noticed
another bug. The semantics of O_EXCL in the sg driver are
modelled on those of a regular file. That means an attempt
to do sg_open(O_EXCL) on a device with an existing open, or
a sg_open() on a device which already has a sg_open(O_EXCL)
active will wait the caller until "the coast is clear".
This assumes O_NONBLOCK is not given, if it is, EBUSY is sent
back. Now that wait needs to be interruptible and was
before Vaughan Cao's patch. In his defence there are no
down_read_interruptible() and down_write_interruptible()
calls available for this purpose.

So I propose the following patch over Vaughan Cao's patch.
It replaces his per-device read-write semaphore with:
   - a normal semaphore (or_sem) to stop co-incident open()s
     and release()s on the same device treading on the same
     data.
   - a wait_queue (open_wait) to handle wait on open() which is
     associated with the use of O_EXCL (when O_NONBLOCK is not
     given). The two wait_event_interruptible() calls are in a
     loop because multiple open()s could be waiting on either
     case. All get woken up but only one will get the down() at
     the bottom of the loop and proceed to complete the open(),
     potentially leaving the other candidates to continue waiting
     until it releases.

Apart from the initializations, all my proposed changes are
in sg_open() and sg_release(). The examples directory in
my sg3_utils version 1.37 package:
    http://sg.danny.cz/sg/sg3_utils.html
contains sg_tst_excl.cpp and sg_tst_excl2.cpp ** which are
designed to stress O_EXCL on the sg driver and friends (bsg
ignores O_EXCL, and the block layer has questionable O_EXCL
semantics). My patch has been successful with these tests and
others on my meagre hardware.

Given that lk 3.12.0 release is not far away, the safest path
may still be to revert Vaughan Cao's patch. I'll leave that
decision to the maintainers.

Doug Gilbert

** C++11 so gcc 4.8 (or late 4.7) needed

View attachment "sg_7vc_dg1.diff" of type "text/x-patch" (4666 bytes)