On 9/26/2018 12:51 PM, Leon Romanovsky wrote:
On Wed, Sep 26, 2018 at 10:34:45AM -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..
Delete all prints from all drivers?
Add lock/unlock for every access to dev_name?
Just kidding, it looks like we will need to leave with this situation,
it is expected to be very rare one and will cause to issues in debug
kernels only, where in theory, it can panic after such access.
In productions kernels without checkers, use-after-free is silently
ignored.
To me, it just isn't worth making a kernel problem worse just to rename
a device to some other identifier. Then again I'm a developer and not a
sys admin so my view is a bit narrow. I won't fight too hard against
this going in.
-Denny
