| Message ID | 20231020011856.3244410-5-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: deduplicate netdev name allocation | expand |
Fri, Oct 20, 2023 at 03:18:54AM CEST, kuba@kernel.org wrote: >Prior to restructuring __dev_alloc_name() handled both printf >and non-printf names. In a clever attempt at code reuse it >always prints the name into a buffer and checks if it's >a duplicate. > >Trust the bitmap, and return an error if its full. > >Signed-off-by: Jakub Kicinski <kuba@kernel.org> >--- > net/core/dev.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > >diff --git a/net/core/dev.c b/net/core/dev.c >index bbfb02b4a228..d2698b4bbad4 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -1119,18 +1119,11 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) > > i = find_first_zero_bit(inuse, max_netdevices); > bitmap_free(inuse); >+ if (i == max_netdevices) >+ return -ENFILE; Hmm, aren't you changeing functionality here? I mean, prior to this patch, the i of value "max_netdevices" was happily used, wan't it? In theory it may break things allowing n-1 netdevices of a name instead of n. > >- snprintf(buf, IFNAMSIZ, name, i); >- if (!netdev_name_in_use(net, buf)) { >- strscpy(res, buf, IFNAMSIZ); >- return i; >- } >- >- /* It is possible to run out of possible slots >- * when the name is long and there isn't enough space left >- * for the digits, or if all bits are used. >- */ >- return -ENFILE; >+ snprintf(res, IFNAMSIZ, name, i); >+ return i; > } > > /* Returns negative errno or allocated unit id (see __dev_alloc_name()) */ >-- >2.41.0 >
On Fri, 20 Oct 2023 12:38:31 +0200 Jiri Pirko wrote: > >+ if (i == max_netdevices) > >+ return -ENFILE; > > Hmm, aren't you changeing functionality here? I mean, prior to this > patch, the i of value "max_netdevices" was happily used, wan't it? > In theory it may break things allowing n-1 netdevices of a name instead > of n. Good point, I should add that to the commit message. But we don't care, right? Nobody is asking to increase the limit, feel like chances that someone will care about 32k vs 32k - 1 devices are extremely low.
Fri, Oct 20, 2023 at 09:04:36PM CEST, kuba@kernel.org wrote: >On Fri, 20 Oct 2023 12:38:31 +0200 Jiri Pirko wrote: >> >+ if (i == max_netdevices) >> >+ return -ENFILE; >> >> Hmm, aren't you changeing functionality here? I mean, prior to this >> patch, the i of value "max_netdevices" was happily used, wan't it? >> In theory it may break things allowing n-1 netdevices of a name instead >> of n. > >Good point, I should add that to the commit message. >But we don't care, right? Nobody is asking to increase >the limit, feel like chances that someone will care >about 32k vs 32k - 1 devices are extremely low. Yes, I think that would be fine. Rare conditions.
diff --git a/net/core/dev.c b/net/core/dev.c index bbfb02b4a228..d2698b4bbad4 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1119,18 +1119,11 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) i = find_first_zero_bit(inuse, max_netdevices); bitmap_free(inuse); + if (i == max_netdevices) + return -ENFILE; - snprintf(buf, IFNAMSIZ, name, i); - if (!netdev_name_in_use(net, buf)) { - strscpy(res, buf, IFNAMSIZ); - return i; - } - - /* It is possible to run out of possible slots - * when the name is long and there isn't enough space left - * for the digits, or if all bits are used. - */ - return -ENFILE; + snprintf(res, IFNAMSIZ, name, i); + return i; } /* Returns negative errno or allocated unit id (see __dev_alloc_name()) */
Prior to restructuring __dev_alloc_name() handled both printf and non-printf names. In a clever attempt at code reuse it always prints the name into a buffer and checks if it's a duplicate. Trust the bitmap, and return an error if its full. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/core/dev.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)