13:15 <ukleinek> arnd: looking at chrdev_open doing fops_get(p->ops); If the module holding p->ops might get be away, evaluating p->ops->owner might oops, doesn't it?

13:17 <ukleinek> related to that, cdev->owner = fops->owner in __register_chrdev is strange

13:21 <arnd> ukleinek: I'm not too familiar with this logic, but I do remember that the entire cdev->owner tracking is done specifically to avoid this race. So I would assume that unloading the module would involve setting i_cdev=NULL under cdev_lock so that any ongoing open() fails, while a successful open increments owner->refcnt to prevent the unload

13:22 <ukleinek> the intention is clear, I still doubt it works

13:37 <ukleinek> hmm, it's hard to construct a failing case to probably it has no practical relevance.

13:45 <arnd> ukleinek: I still don't see an active race in there, I think the comment above the try_module_get() declaration that mentions sysfs should also apply for chrdev: unregistering the character device in this case should block until the device is closed again, and the module does not actually go away until the module_exit() function returns after it has finished unregistering the chrdev

14:02 <ukleinek> arnd: I think the module holding ops might go away before chrdev_open is called

14:10 <arnd> ukleinek: are you thinking of the case where i_cdev is NULL and it does a kobj_lookup to find the cdev, or the case where i_cdev points to an active cdev object?

14:14 <ukleinek> arnd: I don't think about i_cdev at all. I just consider a character device being registered and then the module holding ops going away (and the cdev staying)

14:15 <arnd> I don't understand. If the module gets unloaded and fails to unload the cdev, then you obviously get an instant oops, but that would be a bug in the module_exit function

14:15 <ukleinek> (the module with ops going away is however hard if cdev->owner = THIS_MODULE where THIS_MOUDLE probably holds a reference to the module containing ops)

14:19 <ukleinek> arnd: module A does: somecdev->ops = ops_from_b; cdev_add(somecdev, ...); where ops_from_b is defined in module B. Then rmmod B should work, as the cdev only holds a reference to B once the chardev is opened.

14:20 <ukleinek> B's exit_module functions wouldn't remove somecdev of course

14:22 <arnd> ukleinek: If module A got the ops_from_b from an exported symbol, you wouldn't be able to remove B because of the symbol dependency

14:23 <arnd> if the reference to the ops got passed some other way to module A without taking a reference count, that would be a bug in that module

14:27 <ukleinek> arnd: in that case calling fops_get in chrdev_open() is useless, right?

14:31 <ukleinek> (as just before, cdev_get(p) calls module_get() on p->owner which in this example is A)

14:31 <arnd> ukleinek: I think the fops_get() is used to keep the module pinned until the final close(), while the cdev_get() is paired with cdev_put()

14:32 <ukleinek> cdev_put() isn't called in the success path

14:33 <arnd> it's called before the ops_get()

14:33 <ukleinek> arnd: on new, which is NULL in the success path?

14:34 <ukleinek> hmm, not sure