linux-kernel - Re: [PATCH] regulator: userspace-consumer: Add regulator event support

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] [day] [month] [year] [list]

Message-ID: <ZOfIEzYPU/35Qmtd@finisterre.sirena.org.uk>
Date:   Thu, 24 Aug 2023 22:13:55 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Zev Weiss <zev@...ilderbeest.net>
Cc:     Naresh Solanki <naresh.solanki@...ements.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regulator: userspace-consumer: Add regulator event
 support

On Fri, Aug 04, 2023 at 05:02:02AM -0700, Zev Weiss wrote:
> On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote:
> > On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@...ilderbeest.net> wrote:
> > > On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:

> > > >Add sysfs attribute to track regulator events received from regulator
> > > >notifier block handler.

> > > >+      mutex_lock(&events_lock);
> > > >+      e = data->events;
> > > >+      data->events = 0;

> > > ...particularly this bit -- a read operation on a read-only file (and
> > > especially one with 0644 permissions) having side-effects (clearing the
> > > value it accesses) seems on the face of it like fairly surprising
> > > behavior.  Is this a pattern that's used elsewhere in any other sysfs
> > > files?

> > These are regulator events & are valid when it occurs.
> > Userspace application is intended to consume them as soon as the
> > event is notified by kernel sysfs_notify.

> Sure, but that doesn't really address what I was concerned about -- as
> written this is a read operation on a read-only file (0444, not 0644 as I
> mistakenly wrote above) that nevertheless alters the state of an internal
> kernel data structure.  Can you point to any other sysfs attributes that
> behave like that?  I can't think of one offhand, and I'd be reluctant to
> establish the precedent.

The whole userspace consumer interface is a kludge so I'm not super
concerned about what's effectively clear on read interrupts, ideally
it'd be a file reporting the current status but we don't have a way to
read the current status of everything...

> Would a uevent-based mechanism maybe be a better fit for the problem you're
> trying to solve?

uevents would definitely be good to have, and much better than polling
for apps that can use them, but they don't preclude a read interface.

> > > However, it looks like this would expose the values of all the
> > > REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
> > > something we really want to do?

> > Yes.

> Given that the REGULATOR_EVENT_* constants are defined in headers under
> include/linux and not include/uapi, it doesn't seem like they were intended
> to be used as part of a userspace-visible interface.  If they're going to
> be, I think they should be moved to the uapi directory so that applications
> can use the proper definitions from the kernel instead of manually
> replicating it on their own (but I suspect we should probably find a
> different approach instead).

This is a concern.  We should probably indirect via strings at least,
but that probably implies a file per event at least.  Due to that I'll
drop this patch for this release.  Sorrt for doing that this late, it's
not ideal - like I said in the other thread I lost this thread under a
bunch of others in my inbox.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)