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]

Date:   Mon, 28 Aug 2023 15:05:41 +1000
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 Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > 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.
>
> Nice!
>
> Mind if I take it and clean it up a bit and test with it here?  Again, I
> need this functionality for other stuff as well, so getting it merged
> for your feature is fine with me.

Sure! Go ahead. Sorry I was travelling last week.

>
> > 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
>
> I would rather do it this way.  I can work on this on Friday if you want
> me to.

Yeah, that's fine with me. If you aren't able to let me know and I can
finish up the patch and send it with this series

Alistair

>
> thanks,
>
> greg k-h