pktcdvd oops

Hi Peter,

You don't seem to have a bugzilla account, so could not reassign to you. See http://bugzilla.kernel.org/show_bug.cgi?id=9294

Quoted 4 lines · Jens Axboe

Hi Peter,

You don't seem to have a bugzilla account, so could not reassign to you. See http://bugzilla.kernel.org/show_bug.cgi?id=9294

Problem is repeatable on my computer. It dies in __module_get() on this line:

        BUG_ON(module_refcount(module) == 0);

I think this is because commit 7b595756ec1f49e0049a9e01a1298d53a7faaa15, which states: "Note that with this change, userland holding a sysfs node does not prevent the backing module from being unloaded."

Unfortunately, I don't know how this sysfs stuff is supposed to work, and therefore don't know how to fix the problem.

[Greg cc'd]

Quoted 10 lines · Peter Osterlund

Quoted 4 lines · Jens Axboe

Hi Peter,

You don't seem to have a bugzilla account, so could not reassign to you. See http://bugzilla.kernel.org/show_bug.cgi?id=9294

Problem is repeatable on my computer. It dies in __module_get() on this line:

Quoted 8 lines

        BUG_ON(module_refcount(module) == 0);

I think this is because commit 7b595756ec1f49e0049a9e01a1298d53a7faaa15, which states: "Note that with this change, userland holding a sysfs node does not prevent the backing module from being unloaded."

Unfortunately, I don't know how this sysfs stuff is supposed to work, and therefore don't know how to fix the problem.

Does this fix the problem?

Hello,

have not tested it yet, but i quess, the code mentioned by Peter is in pkt_new_dev() that is called by pkt_setup_dev():

	/* This is safe, since we have a reference from open(). */
		__module_get(THIS_MODULE);

So, now, there must be checks in every sysfs operation in the module code, to ensure that the module is still loaded?

BTW: the bug report says:

Steps to reproduce:

modprobe pktcdvd
echo 22:0 >/sys/class/pktcdvd/add

Is there any module unload??? Why is the module not available after the modprobe, but the sysfs entries, generated by the module? Confused ;)

Quoted 5 lines · Thomas Maier

Hello,

have not tested it yet, but i quess, the code mentioned by Peter is in pkt_new_dev() that is called by pkt_setup_dev():

Quoted 7 lines

    /* This is safe, since we have a reference from open(). */
        __module_get(THIS_MODULE);

So, now, there must be checks in every sysfs operation in the module code,
to ensure that the module is still loaded?

I haven't tested it either yet. What I don't understand is this: If the __module_get() is not safe because the module code could have already been unloaded, how can it possibly be made safe by adding more code to the pktcdvd module? If the module is unloaded, trying to execute its code can't be a good thing no matter what the code does.

Quoted 7 lines

BTW: the bug report says:

Steps to reproduce:

modprobe pktcdvd
echo 22:0 >/sys/class/pktcdvd/add

Quoted 2 lines

Is there any module unload??? Why is the module not available after the modprobe, but the sysfs entries, generated by the module? Confused ;)

I think the purpose of the BUG_ON in __module_get() is to catch cases that are unsafe, even if the call would have happened to work in this particular case.

Quoted 5 lines · Peter Osterlund

If the
__module_get() is not safe because the module code could have already been unloaded, how can it possibly be made safe by adding more code to the pktcdvd module? If the module is unloaded, trying to execute its code can't be a good thing no matter what the code does.

sysfs itself is now out of module lifespan rules. sysfs callbacks are guaranteed to stay in memory while running by sysfs node removal waiting for completion of in-flight operations before returning. In pktcdvd's case, class_destroy() call in pkt_sysfs_cleanup() will wait for all in-flight sysfs r/w ops to complete.

So, even while sysfs callbacks are executing, the module beneath can die but it will stay in memory till all the callbacks return. You need to test module liveness using try_module_get() (and it can fail) if you want to grab module reference from sysfs callbacks.

pkt_setup_dev() expects module reference to be held on invocation. This used to be true for sysfs callbacks but not anymore. Test and grab module reference around pkt_setup_dev() in class_pktcdvd_store_add().

Signed-off-by Tejun Heo <[email protected]>

Acked-by Peter Osterlund <[email protected]>

---
Greg, can you please push this patch through your tree? Thanks a lot.

 drivers/block/pktcdvd.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Quoted 11 lines · Tejun Heo

pkt_setup_dev() expects module reference to be held on invocation. This used to be true for sysfs callbacks but not anymore. Test and grab module reference around pkt_setup_dev() in class_pktcdvd_store_add().

Signed-off-by Tejun Heo <[email protected]>

Acked-by Peter Osterlund <[email protected]>

---
Greg, can you please push this patch through your tree? Thanks a lot.

Quoted 2 lines

 drivers/block/pktcdvd.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Why through my tree? I don't do block devices :)

Shouldn't Jens or at least Andrew take it?

Quoted 15 lines · Greg KH

Quoted 13 lines · Tejun Heo

pkt_setup_dev() expects module reference to be held on invocation. This used to be true for sysfs callbacks but not anymore. Test and grab module reference around pkt_setup_dev() in class_pktcdvd_store_add().

Signed-off-by Tejun Heo <[email protected]>

Acked-by Peter Osterlund <[email protected]>

---
Greg, can you please push this patch through your tree? Thanks a lot.

 drivers/block/pktcdvd.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Quoted 1 line

Why through my tree? I don't do block devices :)

Because it's a regression introduced by changes in sysfs?

Quoted 1 line

Shouldn't Jens or at least Andrew take it?

That's fine too. Jens?