On 2019-03-21 5:36 p.m., Bart Van Assche wrote:
On Wed, 2019-03-20 at 12:39 +0100, Hannes Reinecke wrote:
The 'ch_mutex" is meant to guard against modifications of
file->private_data, so we need to take in in ch_release() as
well as in ch_open().
Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
---
drivers/scsi/ch.c | 2 ﭗ橸ṷ梧뇪觬()
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 1c5051b1c125..8f426903d7e4 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -577,9 +577,11 @@ ch_release(struct inode *inode, struct file *file)
{
scsi_changer *ch = file->private_data;
+ mutex_lock(&ch_mutex);
scsi_device_put(ch->device);
ch->device = NULL;
file->private_data = NULL;
+ mutex_unlock(&ch_mutex);
kref_put(&ch->ref, ch_destroy);
return 0;
}
Hi Hannes,
My interpretation of the open() syscall code (do_sys_open()) is that a new
struct file is allocated every time the open() is called. Does that mean that
the lock and unlock calls for ch_mutex can be removed from ch_open()?
Regarding the above patch: why do you think that file->private_data changes
need to be serialized? I don't know any other file_operations::open / close
callback implementations that implement similar serialization.
Bart,
That doesn't sound right. If it was correct then sg_open() and sg_release()
have mutex overkill (and I don't think that is caused by the complexity of
adding O_EXCL which is damn hard to implement correctly).
Example with existing ch driver code, two threads T1 and T2:
T1 T2
========================================
f1 = open("/dev/ch1")
....
close(f1) f2 = open("dev/sg1")
So if f2=open() gets ch (a pointer) but _before_ it does kref_get(),
close(f1) gets in and does kref_put(&ch->ref, ch_destroy), ref goes
to 0 and ch_destroy() gets called and now ch is dangling ....
Right solution, perhaps Bart could fix the explanation on the patch :-)
Doug Gilbert
P.S. Bart, if your last statement is correct, then they are probably all
broken!
