Messages in this thread | | | Date | Mon, 4 Jul 2022 13:57:56 +0000 | From | Wedson Almeida Filho <> | Subject | Re: [PATCH] char: misc: make misc_open() and misc_register() killable |
| |
On Mon, Jul 04, 2022 at 10:48:32PM +0900, Tetsuo Handa wrote: > On 2022/07/04 21:59, Wedson Almeida Filho wrote: > >> @@ -139,6 +139,10 @@ static int misc_open(struct inode *inode, struct file *file) > >> > >> err = 0; > >> replace_fops(file, new_fops); > >> + if (iter->unlocked_open && file->f_op->open) { > >> + mutex_unlock(&misc_mtx); > >> + return file->f_op->open(inode, file); > >> + } > > > > One of the invariants of miscdev is that once misc_deregister() returns, > > no new calls to f_op->open() are made. (Although, of course, you can > > still have open files but that's a whole different problem.) > > The point of this change is that file->f_op after mutex_unlock(&misc_mtx) is > from new_fops which is guaranteed to hold a ref on "struct file_operations" > via new_fops = fops_get("struct miscdevice"->fops). > That is, a module ref remains valid even after mutex_unlock(&misc_mtx). > > And as with major_names_lock case quoted below, this change assumes that > misc_deregister() is called from module's __exit function, and fops_get() > is preventing the module owning new_fops from calling __exit function.
Your assumption is not sound. misc_deregister() can be (and is) legitimately called from other places, for example, a driver's remove() callback. In fact, when I grep for misc_deregister(), the second instance is such a case.
Anyway, I agree that something should be done about the issue you report, I'm just arguing that this proposal isn't a good one because it breaks an invariant that makes it really hard for drivers to properly cleanup.
> > + if ((*n)->major != major || !(*n)->probe) > + continue; > + if (!try_module_get((*n)->owner)) > + break; > + /* > + * Calling probe function with major_names_lock held causes > + * circular locking dependency problem. Thus, call it after > + * releasing major_names_lock. > + */ > + probe_fn = (*n)->probe; > + mutex_unlock(&major_names_lock); > + /* > + * Assuming that unregister_blkdev() is called from module's > + * __exit function, a module refcount taken above allows us > + * to safely call probe function without major_names_lock held. > + */ > + probe_fn(devt); > + module_put((*n)->owner); > + return; >
| |