On Wed, 2019-01-02 at 09:34 +0000, David Howells wrote: > Jia-Ju Bai <baijiaju1...@gmail.com> wrote: > > > + mutex_lock(&user->file_lock); > > sbefifo_release_command(user); > > free_page((unsigned long)user->cmd_page); > > + mutex_unlock(&user->file_lock); > > It shouldn't be necessary to do the free_page() call inside the locked > section.
True. However, I didn't realize read/write could be concurrent with release so we have another problem. I assume when release is called, no new read/write can be issued, I am correct ? So all we have to protect against is a read/write that has started prior to release being called, right ? In that case, what can happen is that release() wins the race on the mutex, frees everything, then read or write starts using feed stuff. This is nasty to fix because the mutex is in the user structure, so even looking at the mutex is racy if release is called. The right fix, would be, I think, for "user" (pointed to by file- >private_data) to be protected by a kref. That doesn't close it completely as the free in release() can still lead to the structure becoming re-used before read/write tries to get the kref but after it has NULL checked the private data. So to make that solid, I would also RCU-defer the actual freeing and use RCU around dereferencing file->private_data Now, I yet have to see other chardevs do any of the above, do that mean they are all hopelessly racy ? Cheers, Ben.
