re: md/raid10: handle recovery of replacement devices.

re: md/raid10: handle recovery of replacement devices.

[Dan Carpenter]

Hello NeilBrown,

This is a semi-automatic email about new static checker warnings.

The patch e1f6cbf7e1b0: "md/raid10: handle recovery of replacement 
devices." from Nov 9, 2011, leads to the following Smatch complaint:

drivers/md/raid10.c +2814 sync_request()
	 error: we previously assumed 'bio' could be null (see line 2808)

drivers/md/raid10.c
  2807					bio = r10_bio->devs[1].repl_bio;
  2808					if (bio)
                                            ^^^
check.

  2809						bio->bi_end_io = NULL;
  2810					rdev = mirror->replacement;
  2811					if (rdev == NULL ||
  2812					    test_bit(Faulty, &rdev->flags))
  2813						break;
  2814					bio->bi_next = biolist;
                                        ^^^^^^^^^^^^
unconditional dereference.

  2815					biolist = bio;
  2816					bio->bi_private = r10_bio;

regards,
dan carpenter

Re: md/raid10: handle recovery of replacement devices.

[NeilBrown]

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

On Tue, 15 Nov 2011 09:32:39 +0300 Dan Carpenter <dan.carpenter@oracle.com>
wrote:

> Hello NeilBrown,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch e1f6cbf7e1b0: "md/raid10: handle recovery of replacement 
> devices." from Nov 9, 2011, leads to the following Smatch complaint:
> 
> drivers/md/raid10.c +2814 sync_request()
> 	 error: we previously assumed 'bio' could be null (see line 2808)
> 
> drivers/md/raid10.c
>   2807					bio = r10_bio->devs[1].repl_bio;
>   2808					if (bio)
>                                             ^^^
> check.
> 
>   2809						bio->bi_end_io = NULL;
>   2810					rdev = mirror->replacement;
>   2811					if (rdev == NULL ||
>   2812					    test_bit(Faulty, &rdev->flags))
>   2813						break;
>   2814					bio->bi_next = biolist;
>                                         ^^^^^^^^^^^^
> unconditional dereference.
> 
>   2815					biolist = bio;
>   2816					bio->bi_private = r10_bio;
> 
> regards,
> dan carpenter


Thanks.

I happen to know that if mirror->replacement is not NULL, then
conf->have_replacement is true, so r10buf_pool_alloc will have allocated
[1].repl_bio so if we get to 2814, bio will definitely be non-NULL.

Still, it wouldn't hurt to add a test for 'bio' to keep match happy, and put
in a big comment to keep reviewers happy.  I'll consider doing that.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]