Re: [PATCH v2 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 new kernel-doc for parse_mac_pton() in lib/net_utils.c says
  "@s must be MAC_ADDR_STR_LEN characters long", which reads as an…

--

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

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

Link: https://lore.kernel.org/all/20261023110113.2969708-1-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]
Does this kernel-doc match what parse_mac_pton() actually checks? The text
"@s must be MAC_ADDR_STR_LEN characters long" reads as an exact length, but
the code only checks for a minimum length:

lib/net_utils.c:parse_mac_pton() {
    ...
	if (strnlen(s, MAC_ADDR_STR_LEN) < MAC_ADDR_STR_LEN)
		return false;
    ...
}

strnlen() is capped at MAC_ADDR_STR_LEN (17), and the validation loop only
reads s[0] to s[16]. Nothing after the 17th character is looked at. So
inputs like "aa:bb:cc:dd:ee:ff:00", "aa:bb:cc:dd:ee:ffXYZ" or
"aa:bb:cc:dd:ee:ff\n" return true and fill @mac.

The Return: line says false means "@s is malformed". A caller could read
that as saying true means the whole string was a well-formed MAC address.

The old mac_pton() parsed only the prefix in the same way, and existing
callers depend on it. Sysfs and configfs input usually ends in a newline,
and some callers parse a MAC address embedded in a longer string. So the
code looks right and the comment is what is off.

Since this is a newly exported general-purpose API, could the comment say
something like "@s must contain at least MAC_ADDR_STR_LEN characters; only
the first MAC_ADDR_STR_LEN are parsed and any trailing characters are
ignored"? Enforcing an exact length would break existing mac_pton() users.

> +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/20261104025058.2650911-1-liuc63%40xiaopeng.com