Re: [PATCH net-next 1/6] net: mdio: add mdio_device_get() and mdio_device_put()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, May 29, 2023 at 11:34:42PM +0300, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 6:21 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote:
> > > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti:
> 
> ...
> 
> > > > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > > > +{
> > > > +   get_device(&mdiodev->dev);
> > >
> > > Dunno what is the practice in net, but it makes sense to have the pattern as
> > >
> > >       if (mdiodev)
> > >               return to_mdiodev(get_device(&mdiodev->dev));
> > >
> > > which might be helpful in some cases. This approach is used in many cases in
> > > the kernel.
> >
> > The device should exist. If it does not, it is a bug, and the
> > resulting opps will help find the bug.
> 
> The main point in the returned value, the 'if' can be dropped, it's
> not a big deal.

And if you do, that results in a latent bug.

The whole point of doing the above is to cater for the case when
mdiodev is NULL. If mdiodev is NULL, provided "dev" is the first member
of mdiodev, then &mdiodev->dev will also be NULL. That will mean
get_device() will see a NULL pointer and itself return NULL. The
to_mdiodev() will then convert back to a mdiodev which will also be
NULL. So everything works correctly.

If dev is not the first member, then &mdiodev->dev will no longer be
NULL, but will be offset by that amount.

get_device() will check whether that is NULL - it won't be, so it'll
try to pass &dev->kobj to kobject_get(). That will also not be a NULL
pointer. kobject_get() will likely oops the kernel.

So no, it isn't safe to drop that if().

However, count the number of places in the kernel that actually pay
attention to the return value of get_device()...

In drivers/base, which should be the prime area where things are
done correctly, there are 42 places where get_device() is called.
Of those 42 places, 13 places make use of the returned value in
some way, which mean 29 do not bother to check.

Now, what use is checking that return value? Does get_device()
ever return anything different from what was passed in?

Well, we need to delve further down into kobject_get(), which
does not - kobject_get() returns whatever it was given. Note
that kref_get() doesn't return anything, nor does refcount_inc(),
both of which post-date get_device().

So, that in turn means that get_device() will only ever return
what it was passed. So:

	ret = get_device(dev);

`ret' is _guaranteed_ to always be exactly the same was `dev' in
all cases (except if "dev" results in get_device() performing an
invalid dereference and Oopsing the kernel, by which time we won't
care about `ret'.)

Now, if we think about situations where this will be called - they
will always be called where the MDIO device already has reference
counts against it - we have to be holding at least one refcount
to already have a reference to this device. If we don't have that,
then we're in the situation where the memory pointed to by the
mdio device pointer has probably already been freed, and could
already be used for some other purpose - and using the return value
from get_device() isn't going to save you from such a racy stupidity.

If we extend this to mdio_device_get(), then we end up in exactly
the same scenario - and what benefit does it give to the code?
Does it make the code more readable? No, it makes it less readable.
Does it make the code more robust? No, it makes no difference.
Does it make the code more correct? Again, no difference.

So, I don't see any point in changing the proposal I've put forward
as mdio_device_get().

Things would be different if get_device() used kobject_get_unless_zero()
which _can_ alter the returned value, but as I've stated above, if the
refcount were to become zero at the point that mdio_device_get() may
be called, we've *already* lost.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!