On Wed, 2018-09-26 at 10:34 -0600, Jason Gunthorpe wrote:
> On Wed, Sep 26, 2018 at 07:25:10PM +0300, Leon Romanovsky wrote:
> > On Wed, Sep 26, 2018 at 10:02:36AM -0600, Jason Gunthorpe wrote:
> > > On Wed, Sep 26, 2018 at 06:34:23PM +0300, Leon Romanovsky wrote:
> > >
> > > > > So what are the "downsides" to calling this function? I think you should
> > > > > mention that in the commit message and make the justification for why
> > > > > this is OK rather than, someone else did so we can to.
> > > >
> > > > Actually, I reread again the comment above device_rename() and think
> > > > that "downsides" mentioned there can be races with symlinks only.
> > > >
> > > > We are holding lock which prevent addition of new ib_device with same
> > > > name, so from name point of view, we are safe.
> > > >
> > > > Regarding comment, I don't know what else can I add to comment in
> > > > device_rename() section.
> > >
> > > The bigger downside is that all the places that use dev_name will
> > > use after free if they race with device_rename(), but that bug
> > > afflicts netdev to some degree as well, though netdev often (but not
> > > always) uses netdev_name() that doesn't have this race.
> >
> > How? We are holding device_lock which will prevent release of the device.
>
> Because device_rename calls kobject_set_name_vargs which does:
>
> s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> kfree_const(kobj->name);
> kobj->name = s;
>
> And dev_name does:
>
> return kobj->name;
>
> So, it use after frees if you race the two functions.
>
> netdev has this bug, I see no particularly obvious solution..
>
> Jason
I don't think this is the only problem the net stack has in regards to
device name changes. It's supported, but because it's expected to be a
"do it once, early on, then never do it again" thing, they don't worry
too much about the lingering bugs in the name change operations. We
used to have a multiyear bug in IPoIB (maybe still do?) where if you
change the name of an IPoIB device, and you are using debugfs, the
debugfs device name didn't get changed and when you registered your next
IPoIB device you would get warnings about "Unable to create debugfs file
for dev ib0". I don't think we are going to solve those races today, so
if this code works, I see no reason to block it. We just need to know
that if the net folks ever get serious about solving those races, we'll
need to do the same.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
