linux-kernel - Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to attribute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKmqyKM+DNTF1f0FvDEda_db792Ta4w_uAKNTZ6E3NkYoVcPFQ@mail.gmail.com>
Date:   Tue, 22 Aug 2023 16:20:06 -0400
From:   Alistair Francis <alistair23@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        Jonathan.Cameron@...wei.com, lukas@...ner.de,
        alex.williamson@...hat.com, christian.koenig@....com,
        kch@...dia.com, logang@...tatee.com, linux-kernel@...r.kernel.org,
        chaitanyak@...dia.com, rdunlap@...radead.org,
        Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to attribute_group

On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > The documentation for sysfs_merge_group() specifically says
> >
> >     This function returns an error if the group doesn't exist or any of the
> >     files already exist in that group, in which case none of the new files
> >     are created.
> >
> > So just not adding the group if it's empty doesn't work, at least not
> > with the code we currently have. The code can be changed to support
> > this, but it is difficult to determine how this will affect existing use
> > cases.
>
> Did you try?  I'd really really really prefer we do it this way as it's
> much simpler overall to have the sysfs core "do the right thing
> automatically" than to force each and every bus/subsystem to have to
> manually add this type of attribute.
>
> Also, on a personal level, I want this function to work as it will allow
> us to remove some code in some busses that don't really need to be there
> (dynamic creation of some device attributes), which will then also allow
> me to remove some apis in the driver core that should not be used at all
> anymore (but keep creeping back in as there is still a few users.)
>
> So I'll be selfish here and say "please try to get my proposed change to
> work, it's really the correct thing to do here."

I did try!

This is an attempt:
https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9

It is based on your original patch with a bunch of:

if (!parent) {
    parent = kernfs_create_dir_ns(kobj->sd, grp->name,
                  S_IRWXU | S_IRUGO | S_IXUGO,
                  uid, gid, kobj, NULL);
    ...
    }


added throughout the code base.

Very basic testing shows that it does what I need it to do and I don't
see any kernel oops on boot.

I prefer the approach I have in this mailing list patch. But if you
like the commit mentioned above I can tidy and clean it up and then
use that instead

Alistair

>
> thanks,
>
> greg k-h