2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

It looks like the cpuidle patches are doing incorrect things with their kobject protocol.

Begin forwarded message:

Date: Thu, 22 Mar 2007 09:24:38 -0700
From: "Miles Lane" <[email protected]>
To: "Andrew Morton" <[email protected]>
Cc: LKML <[email protected]>, [email protected]
Subject: Re: 2.6.21-rc4-mm1 + 4 hotfixes -- BUG: unable to handle kernel paging request at virtual address 6b6b6ceb -- EIP is at module_put+0x7/0x1f

I cannot reproduce the BUG with your ml.bz2 patch applied. I am seeing this with both 2.6.21-rc4-mm1 + hotfixes, and with 2.6.21-rc4 + ml.bz2:

Mar 22 09:10:35 FractalPath kernel: ACPI: CPU0 (power states: C1[C1]
C2[C2] C3[C3])
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0x40/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0xd4/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: The kobject at, or inside
per_cpu__cpuidle_devices+0x168/0x558 is not dynamically allocated.
Mar 22 09:10:35 FractalPath kernel: cpuidle: using driver acpi_idle

Not sure if this is a problem. Also, the first time I booted the ml.bz2
build, it hung. I don't have netconsole or a serial debugging system set up, so I have no idea what the problem was. The second boot, ipw2200 loaded with no errors, but NetworkManager wouldn't connect until I removed and reinserted the module.

All the best,
Miles
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Andrew Morton

It looks like the cpuidle patches are doing incorrect things with their kobject protocol.

Looks the goal to force kobject to be allocated dynamically is to release the memory of the kobject. But in the cpuidle case, we don't want to free the memory as it might be used soon.

I saw a lot of similar staff too, like 'cpu_devices' in arch/i386/kernel/topology.c.

Quoted 7 lines · Shaohua Li

Quoted 3 lines · Andrew Morton

It looks like the cpuidle patches are doing incorrect things with their kobject protocol.

Looks the goal to force kobject to be allocated dynamically is to release the memory of the kobject. But in the cpuidle case, we don't want to free the memory as it might be used soon.

What do you mean "used soon"? Is this a time critical thing?

Quoted 2 lines

I saw a lot of similar staff too, like 'cpu_devices' in arch/i386/kernel/topology.c.

The point is that kobjects should be created dynamically as they can be referenced by different threads at different times, so you need to let it manage the reference counting logic for you. Don't create them statically, it's almost always wrong.

Note, one valid use of static kobject is in struct bus_type and in some class definitions.

So this should probably be fixed.

drivers/cpuidle/sysfs.c static struct sysfs_ops cpuidle_state_sy · line 279

Quoted 4 lines · Shaohua Li

drivers/cpuidle/sysfs.c +4 -0

+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}

NO!!!

Do people think that I add warnings to the kernel so that people can just work around them by passing empty functions to the driver core?

Sometimes I wonder why I even try...

Please fix this code, it is wrong.

Quoted 7 lines · On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <[email protected]> wrote:

drivers/cpuidle/sysfs.c static struct sysfs_ops cpuidle_state_sy · line 279

Quoted 4 lines · Shaohua Li

drivers/cpuidle/sysfs.c +4 -0

+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}

NO!!!

heh. This happens rather a lot.

Quoted 3 lines

Do people think that I add warnings to the kernel so that people can just work around them by passing empty functions to the driver core?

Quoted 4 lines

Sometimes I wonder why I even try...

Please fix this code, it is wrong.

Is it documented anywhere? I always forget the reasoning so I have to cc you each time I see it happening.

So, where do I need to put this information so that people stop trying to be smarter than the kernel...

Quoted 2 lines · On Tue, 27 Mar 2007 21:51:24 -0700 Greg KH <[email protected]> wrote:

So, where do I need to put this information so that people stop trying to be smarter than the kernel...

Documentation/driver-model/driver.txt?

And a pointer to a sample properly-written driver would help.

Quoted 15 lines · Andrew Morton

Quoted 7 lines · On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <[email protected]> wrote:

drivers/cpuidle/sysfs.c static struct sysfs_ops cpuidle_state_sy · line 279

Quoted 4 lines · Shaohua Li

drivers/cpuidle/sysfs.c +4 -0

+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}

NO!!!

heh. This happens rather a lot.

Quoted 3 lines

Do people think that I add warnings to the kernel so that people can just work around them by passing empty functions to the driver core?

Quoted 7 lines

Quoted 4 lines

Sometimes I wonder why I even try...

Please fix this code, it is wrong.

Is it documented anywhere? I always forget the reasoning so I have to cc you each time I see it happening.

The fact that the kernel itself spits out a message saying:
"Device '%s' does not have a release() function, it is broken and must be fixed."

isn't enough? What should I change it to, something like this:

"Device '%s' does not have a release() function, it is broken and must be fixed, and if you just provide an empty function to remove this warning, rabid echidnas will track you down and puncture your spleen."

The reason for this is that the device is a reference counted object, and you have to free the memory in the release function, otherwise you can easily be accessing memory that is freed already.

Now note, this patch used a completion function to wait for the object to be destroyed, which isn't the nicest. But if you are going to do this, do it in the release function, like the code did a bit earlier.

And if you are doing this because you have two kobjects in the same structure, well, your code is so messed up beyond belief that it needs to be fixed. And yes scsi developers, I'm pointing at you...

Quoted 44 lines · Greg KH

Quoted 15 lines · Andrew Morton

Quoted 7 lines · On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <[email protected]> wrote:

drivers/cpuidle/sysfs.c static struct sysfs_ops cpuidle_state_sy · line 279

Quoted 4 lines · Shaohua Li

drivers/cpuidle/sysfs.c +4 -0

+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}

NO!!!

heh. This happens rather a lot.

Quoted 3 lines

Do people think that I add warnings to the kernel so that people can just work around them by passing empty functions to the driver core?

Quoted 7 lines

Quoted 4 lines

Sometimes I wonder why I even try...

Please fix this code, it is wrong.

Is it documented anywhere? I always forget the reasoning so I have to cc you each time I see it happening.

The fact that the kernel itself spits out a message saying:
"Device '%s' does not have a release() function, it is broken and must be fixed."

isn't enough? What should I change it to, something like this:

"Device '%s' does not have a release() function, it is broken and must be fixed, and if you just provide an empty function to remove this warning, rabid echidnas will track you down and puncture your spleen."

The reason for this is that the device is a reference counted object, and you have to free the memory in the release function, otherwise you can easily be accessing memory that is freed already.

Now note, this patch used a completion function to wait for the object to be destroyed, which isn't the nicest. But if you are going to do this, do it in the release function, like the code did a bit earlier.

Quoted 3 lines

And if you are doing this because you have two kobjects in the same structure, well, your code is so messed up beyond belief that it needs to be fixed. And yes scsi developers, I'm pointing at you...

The two kobjects will be removed in the maintime. As I wait for one object to be destroyed (the two objects are in one structure, and one object's release will free the memory), we haven't the 'using freed memory' issue here. Do we still need a .release for the kobject you pointed out?

Quoted 45 lines · Shaohua Li

Quoted 44 lines · Greg KH

Quoted 15 lines · Andrew Morton

Quoted 7 lines · On Tue, 27 Mar 2007 21:19:58 -0700 Greg KH <[email protected]> wrote:

drivers/cpuidle/sysfs.c static struct sysfs_ops cpuidle_state_sy · line 279

Quoted 4 lines · Shaohua Li

drivers/cpuidle/sysfs.c +4 -0

+static void cpuidle_state_sysfs_release(struct kobject *kobj)
+{
+	/* Nothing required to do here, just workaround kobject warning*/
+}

NO!!!

heh. This happens rather a lot.

Quoted 3 lines

Do people think that I add warnings to the kernel so that people can just work around them by passing empty functions to the driver core?

Quoted 7 lines

Quoted 4 lines

Sometimes I wonder why I even try...

Please fix this code, it is wrong.

Is it documented anywhere? I always forget the reasoning so I have to cc you each time I see it happening.

The fact that the kernel itself spits out a message saying:
"Device '%s' does not have a release() function, it is broken and must be fixed."

isn't enough? What should I change it to, something like this:

"Device '%s' does not have a release() function, it is broken and must be fixed, and if you just provide an empty function to remove this warning, rabid echidnas will track you down and puncture your spleen."

The reason for this is that the device is a reference counted object, and you have to free the memory in the release function, otherwise you can easily be accessing memory that is freed already.

Now note, this patch used a completion function to wait for the object to be destroyed, which isn't the nicest. But if you are going to do this, do it in the release function, like the code did a bit earlier.

Quoted 8 lines

Quoted 3 lines

And if you are doing this because you have two kobjects in the same structure, well, your code is so messed up beyond belief that it needs to be fixed. And yes scsi developers, I'm pointing at you...

The two kobjects will be removed in the maintime. As I wait for one object to be destroyed (the two objects are in one structure, and one object's release will free the memory), we haven't the 'using freed memory' issue here. Do we still need a .release for the kobject you pointed out?

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

struct bar {
	struct foo foo[];

kobject kobjb
}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Quoted 11 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

Quoted 6 lines

struct foo {
	kobject kobja;
}

struct bar {
	struct foo foo[];

Ick, don't do that...

Quoted 7 lines

kobject kobjb
}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

Quoted 16 lines · Greg KH

Quoted 15 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

Quoted 4 lines

Quoted 2 lines

struct bar {
	struct foo foo[];

Ick, don't do that...

why?

Quoted 10 lines

Quoted 7 lines

kobject kobjb
}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

I still didn't get the reason why we couldn't do this in the way of my patch. As I said, there isn't risk to use 'freed memory'. I can make the 'struct foo' a pointer, but this will mess the cpuidle driver.

Thanks,
Shaohua

Quoted 16 lines · Greg KH

Quoted 15 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

Quoted 4 lines

Quoted 2 lines

struct bar {
	struct foo foo[];

Ick, don't do that...

why?

Quoted 1 line

Quoted 1 line

kobject kobjb

Quoted 7 lines

Quoted 6 lines

}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Quoted 2 lines

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

I still didn't get the reason why we couldn't do this in the way of my patch. As I said, there isn't risk to use 'freed memory'. I can make the 'struct foo' a pointer, but this will mess the cpuidle driver.

Quoted 17 lines · Shaohua Li

Quoted 16 lines · Greg KH

Quoted 15 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

struct bar {
	struct foo foo[];

Ick, don't do that...

why?

Quoted 1 line

Quoted 1 line

kobject kobjb

Because you have multiple kobjects in the same object.

It's just that simple, the lifetime rules for such a thing is almost impossible to track properly. Don't do it!

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Quoted 5 lines

Quoted 2 lines

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

I still didn't get the reason why we couldn't do this in the way of my patch. As I said, there isn't risk to use 'freed memory'. I can make the 'struct foo' a pointer, but this will mess the cpuidle driver.

Again, the main point is you can not have more than one reference count for the same structure. It just does not work at all.

So please, fix the code, it is broken.

And yes, I know of other places in the kernel (scsi stack...) that violate this, but that only means that they are wrong, not that it is an excuse for you to do it also.

Quoted 46 lines · Greg KH

Quoted 17 lines · Shaohua Li

Quoted 16 lines · Greg KH

Quoted 15 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

struct bar {
	struct foo foo[];

Ick, don't do that...

why?

Quoted 1 line

Quoted 1 line

kobject kobjb

Because you have multiple kobjects in the same object.

It's just that simple, the lifetime rules for such a thing is almost impossible to track properly. Don't do it!

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Quoted 5 lines

Quoted 2 lines

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

I still didn't get the reason why we couldn't do this in the way of my patch. As I said, there isn't risk to use 'freed memory'. I can make the 'struct foo' a pointer, but this will mess the cpuidle driver.

Again, the main point is you can not have more than one reference count for the same structure. It just does not work at all.

Quoted 5 lines

So please, fix the code, it is broken.

And yes, I know of other places in the kernel (scsi stack...) that violate this, but that only means that they are wrong, not that it is an excuse for you to do it also.

We don't use the kobject to track the reference count.

But anyway, below patch should make you happy.

Quoted 47 lines · Shaohua Li

Quoted 46 lines · Greg KH

Quoted 17 lines · Shaohua Li

Quoted 16 lines · Greg KH

Quoted 15 lines · Shaohua Li

Quoted 8 lines · Greg KH

Putting more than one kobject in the same structure is a broken design. How can you control the lifetime rules properly if there are two reference counts for the same structure? It doesn't work.

If you really need something like this, then just use a pointer to a kobject for one of them instead of embedding it. Why do you need two different kobjects here?

Our data structure is something like below:

struct foo {
	kobject kobja;
}

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

struct bar {
	struct foo foo[];

Ick, don't do that...

why?

Quoted 1 line

Quoted 1 line

kobject kobjb

Because you have multiple kobjects in the same object.

It's just that simple, the lifetime rules for such a thing is almost impossible to track properly. Don't do it!

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

}

kobjb's .release will free struct bar. kobjb is the parent of kobja. if you have a reference on kobja, then kobjb can't be released too, right? So we only kobjb provide a .release to free the memory, kobja's .release isn't required.

Quoted 5 lines

Quoted 2 lines

Why not just use the "normal" parent/child relationship with the kobjects like the rest of the kernel does?

I still didn't get the reason why we couldn't do this in the way of my patch. As I said, there isn't risk to use 'freed memory'. I can make the 'struct foo' a pointer, but this will mess the cpuidle driver.

Again, the main point is you can not have more than one reference count for the same structure. It just does not work at all.

Quoted 6 lines

Quoted 5 lines

So please, fix the code, it is broken.

And yes, I know of other places in the kernel (scsi stack...) that violate this, but that only means that they are wrong, not that it is an excuse for you to do it also.

We don't use the kobject to track the reference count.

Then what do you use it for?

And it doesn't matter if you use it or not, the reference count _is_ used by the kobject and sysfs code, so you have to be aware of it.

Quoted 1 line

But anyway, below patch should make you happy.

I'm still confused as to why you are creating these "extra" kobjects. What are they used for?

But yes, it is "better" in that you are abiding by the reference count rules properly, although your completion handling seems like it might get into a deadlock, but I'll trust you that it works properly :)

Quoted 3 lines · Greg KH

I'm still confused as to why you are creating these "extra" kobjects. What are they used for?

It's for each idle state.

Quoted 5 lines · Shaohua Li

Quoted 3 lines · Greg KH

I'm still confused as to why you are creating these "extra" kobjects. What are they used for?

It's for each idle state.

Where are these states in sysfs? Is this the /sys/devices/system/cpu/cpu0/cache/indexN directories?