On Sun, Oct 07, 2018 at 09:31:27AM +0300, Leon Romanovsky wrote: > On Wed, Oct 03, 2018 at 04:58:13PM -0400, Doug Ledford wrote: > > 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. > > I didn't see any attempts from netdev people to fix mentioned race. They do have a static name field and use it in many places, so a driver written to that API will not have a use after free problem. Jason
