linux-kernel - Re: Can we drop __must_check from driver_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1375894677.7303.55.camel@x61.thuisdomein>
Date:	Wed, 07 Aug 2013 18:57:57 +0200
From:	Paul Bolle <pebolle@...cali.nl>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	David Miller <davem@...emloft.net>, linux-kernel@...r.kernel.org
Subject: Re: Can we drop __must_check from driver_for_each_device()?

On Wed, 2013-08-07 at 14:51 +0900, Greg Kroah-Hartman wrote:
> On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote:
> > On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> > > > 2) Please note that if the callback always returns zero,
> > > > driver_for_each_device() can still return -EINVAL, but only if it was
> > > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> > > > so. Can that actually happen?
> > > 
> > > Possibly.
> > 
> > So driver_for_each_device() really should be NULL "drv" safe.
> 
> Probably not, now that I think about it some more.  I don't see how that
> could ever really happen, do you?

No, I don't.

See there are two groups of user of driver_for_each_device():
1) those that call it on the device_driver member of their
platform_driver, pci_driver, or usb_driver. If that device_driver member
turns out to be NULL we've got bigger problems, I guess;
2) those that call driver_find() to get a pointer to a device_driver.
These users can trivially check for NULL before calling
driver_for_each_device().

[Side note: the comment above driver_find() reads in part:
    This routine provides no locking to prevent the driver it returns
    from being unregistered or unloaded while the caller is using it.
    The caller is responsible for preventing this.

None of these callers of driver_find() seem to be troubled by this.
Their use of the pointer to a device_driver could be racey.]

Anyhow, I think the NULL "dev" check can be dropped.

> > But wouldn't it therefor be better to make sure the callback functions
> > do not return -EINVAL themselves, so -EINVAL will always only indicate
> > the NULL "drv" case?
> 
> I doubt it's a real need at all.

This is, of course, why I raised this issue. Because if we drop the NULL
"drv" check, we have a function that will in most cases always return
zero, because the callbacks it uses mostly return zero. So I think that
if the NULL "drv" check could be dropped, we might as well drop
__must_check here. The few cases were the callback might return non-zero
are already handled correctly.

> > So, since this warning is still there, I'm looking for another way to
> > get rid of it.
> 
> Fix it properly would be good to do, don't you think?

It's hard to disagree with that!

Thanks,

Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/