Curious code in autostart_array

Curious code in autostart_array

[Pete Zaitcev]

Hi, guys:

My copy of 2.6.17-rc5 has the following code in autostart_array():
		mdp_disk_t *desc = sb->disks + i;
		dev_t dev = MKDEV(desc->major, desc->minor);

		if (!dev)
			continue;
		if (dev == startdev)
			continue;
		if (MAJOR(dev) != desc->major || MINOR(dev) != desc->minor)
			continue;

Under what conditions do you think the last if() statement can fire?
What is its purpose? This looks like an attempt to detect bit clipping.
But what exactly?

Cheers,
-- Pete

Re: Curious code in autostart_array

[H. Peter Anvin]

Pete Zaitcev wrote:
> Hi, guys:
> 
> My copy of 2.6.17-rc5 has the following code in autostart_array():
> 		mdp_disk_t *desc = sb->disks + i;
> 		dev_t dev = MKDEV(desc->major, desc->minor);
> 
> 		if (!dev)
> 			continue;
> 		if (dev == startdev)
> 			continue;
> 		if (MAJOR(dev) != desc->major || MINOR(dev) != desc->minor)
> 			continue;
> 
> Under what conditions do you think the last if() statement can fire?
> What is its purpose? This looks like an attempt to detect bit clipping.
> But what exactly?
> 

It can fire if either desc->major or desc->minor overflow the respective 
fields in dev_t.  Unfortunately, it's not guaranteed to do so.

	-hpa

Re: Curious code in autostart_array

[Neil Brown]

On Thursday June 22, zaitcev@redhat.com wrote:
> Hi, guys:
> 
> My copy of 2.6.17-rc5 has the following code in autostart_array():
> 		mdp_disk_t *desc = sb->disks + i;
> 		dev_t dev = MKDEV(desc->major, desc->minor);
> 
> 		if (!dev)
> 			continue;
> 		if (dev == startdev)
> 			continue;
> 		if (MAJOR(dev) != desc->major || MINOR(dev) != desc->minor)
> 			continue;
> 
> Under what conditions do you think the last if() statement can fire?
> What is its purpose? This looks like an attempt to detect bit clipping.
> But what exactly?

First, be aware that this code (which is only available from the
START_ARRAY ioctl) is scheduled to be removed from the kernel next
month.

Second, the code substantially predates my involvement in md, but I
suspect it is simple caution.  desc->major and desc->minor have been
read of the disk, so their values cannot be trusted.
They are both 32 bit and so could easily not be valid major/minor
numbers.  The test slightly reduces the number of silly thing that can
go wrong if the data is bad (though the fact that the superblock is
checksumed makes that fairly unlikely).
(or to put it another way: I don't know either :-)

NeilBrown

Re: Curious code in autostart_array

[Pete Zaitcev]

On Fri, 23 Jun 2006 14:46:13 +1000, Neil Brown <neilb@suse.de> wrote:

> > 		dev_t dev = MKDEV(desc->major, desc->minor);
> > 		if (MAJOR(dev) != desc->major || MINOR(dev) != desc->minor)
> > 			continue;

> desc->major and desc->minor have been
> read of the disk, so their values cannot be trusted.

Oh, right. Sorry.

-- Pete