Re: [PATCH RFC] loop: make partition scanning reliable

[Christoph Hellwig]

On Mon, Jan 26, 2015 at 11:13:28AM -0500, Tejun Heo wrote:
> > @@ -159,12 +159,15 @@ static int blkdev_reread_part(struct block_device *bdev)
> >  		return -EINVAL;
> >  	if (!capable(CAP_SYS_ADMIN))
> >  		return -EACCES;
> > -	if (!mutex_trylock(&bdev->bd_mutex))
> > +	if (!skipbusy)
> > +		mutex_lock(&bdev->bd_mutex);
> > +	else if (!mutex_trylock(&bdev->bd_mutex))
> >  		return -EBUSY;
> 
> Do we actually need the mutex_trylock() path?  Why can't we just
> always grab the mutex?

I wondered about this, too.  The trylock goes all the way back
to when Al first factored out the partition re-reading into common
code ("[PATCH] partition table flush/read cleanup" in the historic tree
converted from bk), and even before that most drivers used a weird
dev_lock_part() consruct operating on a kdev_t, wich looked like a
trylock.

Given that blkdev_reread_part is invoked directly from the ioctl method
without any additional locking it seems fairly pointless for the
case where it's issue from userspace.  In addition to that the loop
driver, nbd and dasd issue it from a driver, as does the root mounting
code for md.

I would be surprised if any of these callers needs it, but a careful
audit would be useful.

I'd also really prefer a patch that makes loop, nbd and dasd call
blkdev_reread_part directly instead of by ioctl_by_bdev as a first
stage preparation, followed by the locking changes  and the changes
to loop.c in separate patch so each does one thing and can be properly
documented.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/