Re: pktcdvd oops

Re: pktcdvd oops

[Peter Osterlund]

On Mon, 5 Nov 2007, Jens Axboe wrote:

> 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.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: pktcdvd oops

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

[Greg cc'd]

Peter Osterlund wrote:
> On Mon, 5 Nov 2007, Jens Axboe wrote:
> 
>> 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.

Does this fix the problem?

-- 
tejun

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 601 bytes --]

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index a8130a4..a5ee213 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -358,10 +358,19 @@ static ssize_t class_pktcdvd_store_add(struct class *c, const char *buf,
 					size_t count)
 {
 	unsigned int major, minor;
+
 	if (sscanf(buf, "%u:%u", &major, &minor) == 2) {
+		/* pkt_setup_dev() expects caller to hold reference to self */
+		if (!try_module_get(THIS_MODULE))
+			return -ENODEV;
+
 		pkt_setup_dev(MKDEV(major, minor), NULL);
+
+		module_put(THIS_MODULE);
+
 		return count;
 	}
+
 	return -EINVAL;
 }
 

Re: pktcdvd oops

[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():

	/* 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 ;)

-Thomas


Am 06.11.2007, 10:06 Uhr, schrieb Tejun Heo <htejun@gmail.com>:

> [Greg cc'd]
>
> Peter Osterlund wrote:
>> On Mon, 5 Nov 2007, Jens Axboe wrote:
>>
>>> 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.
>
> Does this fix the problem?
>

Re: pktcdvd oops

[Peter Osterlund]

On Tue, 6 Nov 2007, Thomas Maier wrote:

> 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?

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.

> 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 ;)

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.

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: pktcdvd oops

[Tejun Heo]

Peter Osterlund wrote:
> On Tue, 6 Nov 2007, Thomas Maier wrote:
> 
>> 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?
>
> 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.
>

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.

>> 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 ;)
> 
> 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.

The BUG_ON is detecting valid condition here.  If you rmmod pktcdvd
after sysfs write has begun but before __module_get() ran, device node
will be created after the module is killed and scheduled to be unloaded.

Thanks.

-- 
tejun

Re: pktcdvd oops

[Peter Osterlund]

On Wed, 7 Nov 2007, Tejun Heo wrote:

> Peter Osterlund wrote:
>> 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.

Thanks for the explanation.

Given that explanation, I think the patch is correct and it does fix the 
BUG on my computer. Can you please push it upstream?

In any case:

Acked-by: Peter Osterlund <petero2@telia.com>

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

[PATCH] pktcdvd: fix BUG caused by sysfs module reference semantics change

[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 <htejun@gmail.com>
Acked-by: Peter Osterlund <petero2@telia.com>
---
Greg, can you please push this patch through your tree? 
Thanks a lot.

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

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index a8130a4..a5ee213 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -358,10 +358,19 @@ static ssize_t class_pktcdvd_store_add(struct class *c, const char *buf,
 					size_t count)
 {
 	unsigned int major, minor;
+
 	if (sscanf(buf, "%u:%u", &major, &minor) == 2) {
+		/* pkt_setup_dev() expects caller to hold reference to self */
+		if (!try_module_get(THIS_MODULE))
+			return -ENODEV;
+
 		pkt_setup_dev(MKDEV(major, minor), NULL);
+
+		module_put(THIS_MODULE);
+
 		return count;
 	}
+
 	return -EINVAL;
 }
 

Re: [PATCH] pktcdvd: fix BUG caused by sysfs module reference semantics change

[Greg KH]

On Thu, Nov 08, 2007 at 11:27:16AM +0900, Tejun Heo wrote:
> 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 <htejun@gmail.com>
> Acked-by: Peter Osterlund <petero2@telia.com>
> ---
> Greg, can you please push this patch through your tree? 
> Thanks a lot.
> 
>  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?

thanks,

greg k-h

Re: [PATCH] pktcdvd: fix BUG caused by sysfs module reference semantics change

[Tejun Heo]

Greg KH wrote:
> On Thu, Nov 08, 2007 at 11:27:16AM +0900, Tejun Heo wrote:
>> 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 <htejun@gmail.com>
>> Acked-by: Peter Osterlund <petero2@telia.com>
>> ---
>> Greg, can you please push this patch through your tree? 
>> Thanks a lot.
>>
>>  drivers/block/pktcdvd.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
> 
> Why through my tree?  I don't do block devices :)

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

> Shouldn't Jens or at least Andrew take it?

That's fine too.  Jens?

-- 
tejun

Re: [PATCH] pktcdvd: fix BUG caused by sysfs module reference semantics change

[Jens Axboe]

On Thu, Nov 08 2007, Tejun Heo wrote:
> Greg KH wrote:
> > On Thu, Nov 08, 2007 at 11:27:16AM +0900, Tejun Heo wrote:
> >> 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 <htejun@gmail.com>
> >> Acked-by: Peter Osterlund <petero2@telia.com>
> >> ---
> >> Greg, can you please push this patch through your tree? 
> >> Thanks a lot.
> >>
> >>  drivers/block/pktcdvd.c |    9 +++++++++
> >>  1 file changed, 9 insertions(+)
> > 
> > Why through my tree?  I don't do block devices :)
> 
> Because it's a regression introduced by changes in sysfs?
> 
> > Shouldn't Jens or at least Andrew take it?
> 
> That's fine too.  Jens?

Sure, I'm pushing some stuff off today anyway.

-- 
Jens Axboe