Skip to contentSkip to conversation

md: fix multipath oops

PATCH In review Andre Noll 2 participants 3 messages Nov 17, 2008 – Nov 21, 2008 MD RAID

lore
Share & tools
lore

Andre Noll

author
#0 lore raw

PATCH 1/1 md: fix multipath oops

Hi

the current for-next kernel oopses on the 00multipath test of the mdadm test suite. I'm not sure if the patch below is the best way to fix this problem, but it avoids the oops and makes the 00multipath test succeed. In the long run it might be preferable to use a wrapper for sysfs_notify_dirent() that checks for NULL pointers, or even teach sysfs_notify_dirent() to return early if it gets passed a NULL pointer.

Please consider for inclusion.

Thanks
Andre

commit 9448a199ff0e4706ff8146f889bf5c3ff96dee7a

Author: Andre Noll <[email protected]>
Date:   Mon Nov 17 11:36:19 2008 +0100

md: Avoid NULL pointer dereference in sysfs_notify_dirent().

The 00multipath test of the mdadm test suite causes the oops below in sysfs_notify_dirent() due to mddev->sysfs_action being NULL in md_check_recovery().

Check if mddev->sysfs_action is NULL and do not call sysfs_notify_dirent() in this case.

Signed-off-by Andre Noll <[email protected]>
drivers/md/md.c +2 -2
index 61a87f4..7c5617d 100644
@@ -6233,8 +6233,8 @@ void md_check_recovery(mddev_t *mddev)
 	unlock:
 		if (!mddev->sync_thread) {
 			clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
-			if (test_and_clear_bit(MD_RECOVERY_RECOVER,
-					       &mddev->recovery))
+			if (mddev->sysfs_action && test_and_clear_bit(
+				MD_RECOVERY_RECOVER, &mddev->recovery))
 				sysfs_notify_dirent(mddev->sysfs_action);
 		}
 		mddev_unlock(mddev);
Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
#1 lore raw
Quoted 8 lines · On Monday November 17, [email protected] wrote:

Hi

the current for-next kernel oopses on the 00multipath test of the mdadm test suite. I'm not sure if the patch below is the best way to fix this problem, but it avoids the oops and makes the 00multipath test succeed. In the long run it might be preferable to use a wrapper for sysfs_notify_dirent() that checks for NULL pointers, or even teach sysfs_notify_dirent() to return early if it gets passed a NULL pointer.

Hi Andre,
Thanks for this.
I thought I had fixed that already, but maybe I hadn't pushed the tree out. I have now.

BTW, I have two problems with your patch as it stands.

drivers/md/md.c void md_check_recovery(mddev_t *mddev) · line 6239

Quoted 10 lines
drivers/md/md.c +2 -2
@@ -6233,8 +6233,8 @@ void md_check_recovery(mddev_t *mddev)
 	unlock:
 		if (!mddev->sync_thread) {
 			clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
-			if (test_and_clear_bit(MD_RECOVERY_RECOVER,
-					       &mddev->recovery))
+			if (mddev->sysfs_action && test_and_clear_bit(
+				MD_RECOVERY_RECOVER, &mddev->recovery))
 				sysfs_notify_dirent(mddev->sysfs_action);
 		}

1/ The indenting is not what I like. Having the arguments to a function further left than the function name makes is hard to read. I would prefer

			if (mddev->sysfs_action &&
			    test_and_clear_bit(MD_RECOVERY_RECOVER,
					       &mddev->recovery))
				sysfs_notify_dirent(mddev->sysfs_action);

which is one more line, but worth it.

2/ You have introduced a subtle semantic change. If ->sys_action is
NULL, we no longer clear MD_RECOVERY_RECOVER. I suspect this isn't actually an error in practice, but I think it is still the wrong way around to do the tests.
I have

			if (test_and_clear_bit(MD_RECOVERY_RECOVER,
					       &mddev->recovery))
				if (mddev->sysfs_action)
					sysfs_notify_dirent(mddev->sysfs_action);

Thanks,
NeilBrown

author
#2 lore raw
Quoted 9 lines · Neil Brown

2/ You have introduced a subtle semantic change. If ->sys_action is
NULL, we no longer clear MD_RECOVERY_RECOVER. I suspect this isn't actually an error in practice, but I think it is still the wrong way around to do the tests.
I have

			if (test_and_clear_bit(MD_RECOVERY_RECOVER,
					       &mddev->recovery))
				if (mddev->sysfs_action)
					sysfs_notify_dirent(mddev->sysfs_action);

Yes, I agree that's more reasonable.

Thanks
Andre

Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe