On 2019-03-21 7:11 p.m., Douglas Gilbert wrote:
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 <[email protected]> --- 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")
That should be close(f1) colliding with f2=open("dev/ch1")
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!
