On Fri 27-11-20 10:48:42, Christoph Hellwig wrote:
> On Thu, Nov 26, 2020 at 07:22:19PM +0100, Jan Kara wrote:
> > On Thu 26-11-20 14:04:15, Christoph Hellwig wrote:
> > > struct hd_struct *disk_get_part(struct gendisk *disk, int partno)
> > > {
> > > - struct hd_struct *part;
> > > + struct block_device *part;
> > >
> > > rcu_read_lock();
> > > part = __disk_get_part(disk, partno);
> > > - if (part)
> > > - get_device(part_to_dev(part));
> > > - rcu_read_unlock();
> > > + if (!part) {
> > > + rcu_read_unlock();
> > > + return NULL;
> > > + }
> > >
> > > - return part;
> > > + get_device(part_to_dev(part->bd_part));
> > > + rcu_read_unlock();
> > > + return part->bd_part;
> > > }
> >
> > This is not directly related to this particular patch but I'm wondering:
> > What prevents say del_gendisk() from racing with disk_get_part(), so that
> > delete_partition() is called just after we fetched 'part' pointer and the
> > last 'part' kobject ref is dropped before disk_get_part() calls
> > get_device()? I don't see anything preventing that and so we'd hand out
> > 'part' that is soon to be freed (after RCU grace period expires).
>
> At this point the hd_struct is already allocated together with the
> block_device, and thus only freed after the last block_device reference
> goes away plus the inode freeing RCU grace period. So the device model
> ref to part is indeed gone, but that simply does not matter any more.
Well, but once device model ref to part is gone, we're going to free the
bdev inode ref as well. Thus there's nothing which pins the bdev containing
hd_struct?
But now as I'm thinking about it you later switch the device model reference
to just pure inode reference and use igrab() which will reliably return
NULL if the inode is on it's way to be destroyed so probably we are safe in
the final state.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
