Re: [PATCH v4 1/2] lib/net_utils: allow mac_pton() to take a custom delimiter

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The kernel-doc this patch adds for parse_mac_pton() in
  lib/net_utils.c does not match what the function does.

--

lib/net_utils: allow mac_pton() to take a custom delimiter

This commit moves the body of mac_pton() into a new parse_mac_pton(),
which takes a string of accepted separator characters. mac_pton()
becomes a static inline wrapper that passes ":", so existing callers
behave as before.

Link: https://lore.kernel.org/all/20261105072147.3383565-2-liuc63@xxxxxxxxxxxx/

> diff --git a/lib/net_utils.c b/lib/net_utils.c
> index 215cda672fee1..011857cc978b4 100644
> --- a/lib/net_utils.c
> +++ b/lib/net_utils.c
> @@ -5,7 +5,18 @@
>  #include <linux/export.h>
>  #include <linux/hex.h>
>  
> -bool mac_pton(const char *s, u8 *mac)
> +/**
> + * parse_mac_pton - convert a MAC address string using a custom delimiter
> + * @s: string to be parsed
> + * @mac: buffer to store the result, left untouched if @s is malformed
> + * @delim: characters accepted as a separator between the byte pairs
> + *
> + * @s must be MAC_ADDR_STR_LEN characters long and use a character from
> + * @delim at every separator position, e.g. "aa.bb:cc.dd:ee.ff" with ".:".
> + *
> + * Return: true on success, false if @s is malformed.
> + */

[Severity: Low]
This isn't a bug, but does this kernel-doc describe what parse_mac_pton()
actually does?

The only length check in the function is:

	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;

strnlen() is capped at MAC_ADDR_STR_LEN, so this check only rejects
strings that are too short. The parsing loops read only s[0] through
s[16], and nothing checks that s[MAC_ADDR_STR_LEN] is '\0'.

So input such as "aa:bb:cc:dd:ee:ffJUNK", or the 7-octet
"aa:bb:cc:dd:ee:ff:00", returns true and fills @mac with the first six
octets. That doesn't fit "@s must be MAC_ADDR_STR_LEN characters long"
or "false if @s is malformed".

The runtime behaviour is the same as the old mac_pton(). The current
callers (the mac_pton() wrapper, and get_ether_addr() in u_ether.c later
in this series) don't depend on long strings being rejected. However, a
new caller that trusts this doc could skip its own end-of-string check.

Could the doc say instead that @s must have at least MAC_ADDR_STR_LEN
characters and that anything after them is ignored? The other option is
to reject strings where s[MAC_ADDR_STR_LEN] != '\0', but that would
change behaviour for existing mac_pton() callers.

> +bool parse_mac_pton(const char *s, u8 *mac, const char *delim)
>  {
>  	int i;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106030605.576584-1-liuc63%40xiaopeng.com