| Message ID | 20210318034253.w4w2p3kvi4m6vqp5@dwarf.suse.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c015a2256801597fadcbc11d287774c9c512fa5 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: check all name nodes in __dev_alloc_name | expand |
On Thu, 18 Mar 2021 04:42:53 +0100 Jiri Bohac <jbohac@suse.cz> wrote: > for_each_netdev(net, d) { > + struct netdev_name_node *name_node; > + list_for_each_entry(name_node, &d->name_node->list, list) { > + if (!sscanf(name_node->name, name, &i)) > + continue; > + if (i < 0 || i >= max_netdevices) > + continue; > + > + /* avoid cases where sscanf is not exact inverse of printf */ > + snprintf(buf, IFNAMSIZ, name, i); > + if (!strncmp(buf, name_node->name, IFNAMSIZ)) > + set_bit(i, inuse); > + } Rather than copy/paste same code two places, why not make a helper function?
Thu, Mar 18, 2021 at 04:42:53AM CET, jbohac@suse.cz wrote: >__dev_alloc_name(), when supplied with a name containing '%d', >will search for the first available device number to generate a >unique device name. > >Since commit ff92741270bf8b6e78aa885f166b68c7a67ab13a ("net: >introduce name_node struct to be used in hashlist") network >devices may have alternate names. __dev_alloc_name() does take >these alternate names into account, possibly generating a name >that is already taken and failing with -ENFILE as a result. > >This demonstrates the bug: > > # rmmod dummy 2>/dev/null > # ip link property add dev lo altname dummy0 > # modprobe dummy numdummies=1 > modprobe: ERROR: could not insert 'dummy': Too many open files in system > >Instead of creating a device named dummy1, modprobe fails. > >Fix this by checking all the names in the d->name_node list, not just d->name. > >Signed-off-by: Jiri Bohac <jbohac@suse.cz> >Fixes: ff92741270bf ("net: introduce name_node struct to be used in hashlist") Reviewed-by: Jiri Pirko <jiri@nvidia.com> Thanks!
On Wed, Mar 17, 2021 at 09:11:08PM -0700, Stephen Hemminger wrote:
> Rather than copy/paste same code two places, why not make a helper function?
I tried and in it was ugly (too many dependencies into the
currecnt function)
Another option I considered and scratched was to opencode and
modify list_for_each to also act on the dev->name_node
which contains the list head. Or maybe one of the
list_for_each_* variants could be directly misused for that.
I don't understand why this has been designed in such a
non-standard way; why is the first node not part of the list and
the head directly in the net_device?
In the end I considered the copy'n'paste of 9 lines the least
ugly and most readable.
On Thu, 18 Mar 2021 10:06:52 +0100 Jiri Bohac <jbohac@suse.cz> wrote: > On Wed, Mar 17, 2021 at 09:11:08PM -0700, Stephen Hemminger wrote: > > Rather than copy/paste same code two places, why not make a helper function? > > I tried and in it was ugly (too many dependencies into the > currecnt function) > > Another option I considered and scratched was to opencode and > modify list_for_each to also act on the dev->name_node > which contains the list head. Or maybe one of the > list_for_each_* variants could be directly misused for that. That seems like overly complex and unhelpful option. > I don't understand why this has been designed in such a > non-standard way; why is the first node not part of the list and > the head directly in the net_device? > > In the end I considered the copy'n'paste of 9 lines the least > ugly and most readable. > Sure, make sense.
On Thu, Mar 18, 2021 at 04:42:53AM +0100, Jiri Bohac wrote: > __dev_alloc_name(), when supplied with a name containing '%d', > will search for the first available device number to generate a > unique device name. > > Since commit ff92741270bf8b6e78aa885f166b68c7a67ab13a ("net: > introduce name_node struct to be used in hashlist") network > devices may have alternate names. __dev_alloc_name() does take Should this be "does not take" > these alternate names into account, possibly generating a name > that is already taken and failing with -ENFILE as a result. Andrew
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Thu, 18 Mar 2021 04:42:53 +0100 you wrote: > __dev_alloc_name(), when supplied with a name containing '%d', > will search for the first available device number to generate a > unique device name. > > Since commit ff92741270bf8b6e78aa885f166b68c7a67ab13a ("net: > introduce name_node struct to be used in hashlist") network > devices may have alternate names. __dev_alloc_name() does take > these alternate names into account, possibly generating a name > that is already taken and failing with -ENFILE as a result. > > [...] Here is the summary with links: - net: check all name nodes in __dev_alloc_name https://git.kernel.org/netdev/net/c/6c015a225680 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/core/dev.c b/net/core/dev.c index 6c5967e80132..7cbcd8d37e91 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1184,6 +1184,18 @@ static int __dev_alloc_name(struct net *net, const char *name, char *buf) return -ENOMEM; for_each_netdev(net, d) { + struct netdev_name_node *name_node; + list_for_each_entry(name_node, &d->name_node->list, list) { + if (!sscanf(name_node->name, name, &i)) + continue; + if (i < 0 || i >= max_netdevices) + continue; + + /* avoid cases where sscanf is not exact inverse of printf */ + snprintf(buf, IFNAMSIZ, name, i); + if (!strncmp(buf, name_node->name, IFNAMSIZ)) + set_bit(i, inuse); + } if (!sscanf(d->name, name, &i)) continue; if (i < 0 || i >= max_netdevices)
__dev_alloc_name(), when supplied with a name containing '%d', will search for the first available device number to generate a unique device name. Since commit ff92741270bf8b6e78aa885f166b68c7a67ab13a ("net: introduce name_node struct to be used in hashlist") network devices may have alternate names. __dev_alloc_name() does take these alternate names into account, possibly generating a name that is already taken and failing with -ENFILE as a result. This demonstrates the bug: # rmmod dummy 2>/dev/null # ip link property add dev lo altname dummy0 # modprobe dummy numdummies=1 modprobe: ERROR: could not insert 'dummy': Too many open files in system Instead of creating a device named dummy1, modprobe fails. Fix this by checking all the names in the d->name_node list, not just d->name. Signed-off-by: Jiri Bohac <jbohac@suse.cz> Fixes: ff92741270bf ("net: introduce name_node struct to be used in hashlist")