[PATCH] md: fix multipath oops

[PATCH] md: fix multipath oops

[Andre Noll]

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

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 <maan@systemlinux.org>
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 <maan@systemlinux.org>

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 61a87f4..7c5617d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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);
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] md: fix multipath oops

[Neil Brown]

On Monday November 17, maan@systemlinux.org 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.
> @@ -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

Re: [PATCH] md: fix multipath oops

[Andre Noll]

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

On 08:57, Neil Brown wrote:
> 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
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]