Re: [PATCH net-next 1/2] sysfs: facilitate creating an empty directory

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch silently reverses the semantics that commit
  a91845b9a872 ("sysfs: Skip empty folders creation") set up on purpose…
- [Low] The kernel-doc for struct attribute_group in
  include/linux/sysfs.h (lines 101-103) still says "@bin_attrs: ...…
- [Low] The comment and debug message on the rewritten check in
  internal_create_group() (fs/sysfs/group.c:144-147) are misleading.

--

sysfs: facilitate creating an empty directory

This patch changes the early return in internal_create_group() so that it
only skips groups with no name and no attrs or bin_attrs. A named group with
both arrays NULL now gets an empty sysfs directory. Callers no longer need an
attribute list that holds only the sentinel entry.

> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 4e1e4f18a1669..031cb6f36b5b0 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -141,9 +141,10 @@ static int internal_create_group(struct kobject *kobj, int update,
>  	if (unlikely(update && !kobj->sd))
>  		return -EINVAL;
>  
> -	if (!grp->attrs && !grp->bin_attrs) {
> -		pr_debug("sysfs: (bin_)attrs not set by subsystem for group: %s/%s, skipping\n",
> -			 kobj->name, grp->name ?: "");
> +	/* If name is set w/o attributes, create an empty directory */
> +	if (!grp->name && !grp->attrs && !grp->bin_attrs) {

[Severity: Medium]
This reverses behaviour that commit a91845b9a872 ("sysfs: Skip empty
folders creation") added on purpose. Was that intended?

That commit's message covers a group "with a name and without .[bin_]attrs
members". It says sysfs should "just skip the attribute group creation". It
also calls an empty sysfs folder "disturbing".

After this change, a named group with attrs and bin_attrs both NULL always
gets a directory. That applies to sysfs_create_group(), sysfs_create_groups()
and device_add_group(). It also applies to sysfs_update_group() when the
directory doesn't exist yet.

__first_visible() returns 0 when both arrays are NULL. So mode becomes
S_IRWXU | S_IRUGO | S_IXUGO and the code reaches:

    kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
                              uid, gid, kobj, NULL);
    if (IS_ERR(kn)) {
        if (PTR_ERR(kn) == -EEXIST)
            sysfs_warn_dup(kobj->sd, grp->name);
        return PTR_ERR(kn);
    }

A name clash used to return 0 quietly. It now returns -EEXIST and calls
sysfs_warn_dup(), and internal_create_groups() then rolls back the whole
group list.

Some named groups are declared with NULL attrs and filled in at run time.
One example is x86_pmu_format_group in arch/x86/events/core.c:

    static struct attribute_group x86_pmu_format_group __ro_after_init = {
        .name = "format",
        .attrs = NULL,
    };

I couldn't find an in-tree user that registers such a group with the arrays
still NULL. The x86 pmu, s390 PAI, amdgpu, i915, arm-cci and
nvmem_populate_sysfs_cells() users all fill the arrays or bail out first.
Even so, the commit message describes this only as changing one check.

Could the commit message reference a91845b9a872 and explain why skipping
empty named groups is no longer wanted? Could it also say whether existing
named groups with runtime-filled attrs were checked?

[Severity: Low]
A named group with both arrays NULL is now a supported construct. However,
the kernel-doc for struct attribute_group in include/linux/sysfs.h still
says:

 * @attrs:	Pointer to NULL terminated list of attributes.
 * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
 *		Either attrs or bin_attrs or both must be provided.

Should this be updated too? It could say that a named group with neither
array creates an empty directory, and only an unnamed one is skipped.

The follow-up change for wireless_group in net/core/net-sysfs.c depends on
this, and it doesn't touch the header either.

> +		pr_debug("sysfs: (bin_)attrs not set by subsystem for group: %s, skipping\n",
> +			 kobj->name);

[Severity: Low]
This isn't a bug, but the new comment describes what happens when the
condition is false. The block below it actually skips an unnamed group that
has no attributes.

The pr_debug() also prints kobj->name after "for group:". grp->name is
always NULL in this branch, so the printed value is the kobject name, not a
group name. The old "%s/%s" format kept the two separate.

There is also a typo in the commit message: "sentinal" should be "sentinel".

>  		return 0;
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0a0a7987-abb2-49c8-bc16-3938233ae9c5%40gmail.com