On Fri, 2019-01-04 at 10:26 +0800, Jia-Ju Bai wrote: > > On 2019/1/4 8:47, Benjamin Herrenschmidt wrote: > > On Wed, 2018-12-26 at 21:56 +0800, Jia-Ju Bai wrote: > > > In drivers/fsi/fsi-sbefifo.c, the functions sbefifo_user_release(), > > > sbefifo_user_read() and sbefifo_user_write() may be concurrently executed. > > So after refreshing my mind, looking at the code and talking with Al, I > > really dont' see what race you are trying to fix here. > > > > read/write should never be concurrent with release for a given file and > > the stuff we are protecting here is local to the file instance. > > > > Do you have an actual problem you observed ? > > > > Thanks for the reply. > > In fact, this report is found by a static tool written by myself, > instead of real execution. > My tool found that in some drivers, for the structure "struct > file_operations", the code in intetrfaces ".read" , "write" and > ".release" are protected by the same lock. > The functions kcs_bmc_read(), kcs_bmc_write() and kcs_bmc_release() are > examples. > Thus, my tool inferred that the intetrfaces ".read" , "write" and > ".release" of "struct file_operations" can be concurrently executed, and > generated this report. > I manually checked this report, but I was not very sure of it, so I > marked it as a "possible bug" and reported it.
So what happens is that they cannot be executed concurrently for a given struct file. But they can be for separate files. In the fsi-sbefifo case, all of the data and the lock are part of a private structure which is allocated in open() and thus is per-file instance, so there should be no race. In the example you gave, kcs_bmc.c, the data and lock are part of a per-device (struct kcs_bmc) and thus shared by all file instances. So in that case, the race does exist. > From your message, now I know my report is false, and ".read" , "write" > cannot be concurrently executed with ".release" for a given file. > Sorry for my false report, and thanks for your message. Right, your tool is valuable as pre-screening but you need in addition to check (probably manually) whether the data accessed (and lock) are shared by multiple open file instances or are entirely local to a given file instance. Cheers, Ben.
