| Message ID | 20200622100056.10151-4-brgl@bgdev.pl |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: improve devres helpers | expand |
On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > This devres helper registers a release callback that only unregisters > the net_device. It works perfectly fine with netdev structs that are > not managed on their own. There's no reason to check this - drop the > warning. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> I think the reasoning for this suggestion was to catch possible UAF errors. The netdev doesn't necessarily has to be from devm_alloc_* but it has to be part of devm-ed memory or memory which is freed after driver's remove callback. Are there cases in practice where you've seen the netdev not being devm allocated?
wt., 23 cze 2020 o 00:49 Jakub Kicinski <kuba@kernel.org> napisał(a): > > On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > This devres helper registers a release callback that only unregisters > > the net_device. It works perfectly fine with netdev structs that are > > not managed on their own. There's no reason to check this - drop the > > warning. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > I think the reasoning for this suggestion was to catch possible UAF > errors. The netdev doesn't necessarily has to be from devm_alloc_* > but it has to be part of devm-ed memory or memory which is freed > after driver's remove callback. > Yes I understand that UAF was the concern here, but this limitation is unnecessary. In its current form devm_register_netdev() only works for struct net_device allocated with devm_alloc_etherdev(). Meanwhile calling alloc_netdev() (which doesn't have its devm counterpart yet - I may look into it shortly), then registering a devm action with devm_add_action_or_reset() which would free this memory is a perfectly fine use case. This patch would make it possible. > Are there cases in practice where you've seen the netdev not being > devm allocated? As I said above - alloc_netdev() used by wireless, can, usb etc. drivers doesn't have a devres variant. Bartosz
On Tue, 23 Jun 2020 11:12:24 +0200 Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 00:49 Jakub Kicinski <kuba@kernel.org> napisał(a): > > On Mon, 22 Jun 2020 12:00:48 +0200 Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > This devres helper registers a release callback that only unregisters > > > the net_device. It works perfectly fine with netdev structs that are > > > not managed on their own. There's no reason to check this - drop the > > > warning. > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > I think the reasoning for this suggestion was to catch possible UAF > > errors. The netdev doesn't necessarily has to be from devm_alloc_* > > but it has to be part of devm-ed memory or memory which is freed > > after driver's remove callback. > > > > Yes I understand that UAF was the concern here, but this limitation is > unnecessary. In its current form devm_register_netdev() only works for > struct net_device allocated with devm_alloc_etherdev(). Meanwhile > calling alloc_netdev() (which doesn't have its devm counterpart yet - > I may look into it shortly), If resource managed alloc_netdev() is needed devm_alloc_netdev() can be created, and even reuse devm_free_netdev() so no changes to the warning are even necessary for such extension. > then registering a devm action with devm_add_action_or_reset() which > would free this memory is a perfectly fine use case. This patch would > make it possible. alloc_netdev() + devm_add_action makes no sense in the upstream kernel, just add the appropriate helper, we care little about out of tree code. > > Are there cases in practice where you've seen the netdev not being > > devm allocated? > > As I said above - alloc_netdev() used by wireless, can, usb etc. > drivers doesn't have a devres variant.
diff --git a/net/devres.c b/net/devres.c index 57a6a88d11f6..1583ccb207c0 100644 --- a/net/devres.c +++ b/net/devres.c @@ -46,14 +46,6 @@ static void devm_netdev_release(struct device *dev, void *this) unregister_netdev(res->ndev); } -static int netdev_devres_match(struct device *dev, void *this, void *match_data) -{ - struct net_device_devres *res = this; - struct net_device *ndev = match_data; - - return ndev == res->ndev; -} - /** * devm_register_netdev - resource managed variant of register_netdev() * @dev: managing device for this netdev - usually the parent device @@ -61,22 +53,13 @@ static int netdev_devres_match(struct device *dev, void *this, void *match_data) * * This is a devres variant of register_netdev() for which the unregister * function will be call automatically when the managing device is - * detached. Note: the net_device used must also be resource managed by - * the same struct device. + * detached. */ int devm_register_netdev(struct device *dev, struct net_device *ndev) { struct net_device_devres *dr; int ret; - /* struct net_device must itself be managed. For now a managed netdev - * can only be allocated by devm_alloc_etherdev_mqs() so the check is - * straightforward. - */ - if (WARN_ON(!devres_find(dev, devm_free_netdev, - netdev_devres_match, ndev))) - return -EINVAL; - dr = devres_alloc(devm_netdev_release, sizeof(*dr), GFP_KERNEL); if (!dr) return -ENOMEM;