* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sirius @ 2024-04-28 4:25 UTC (permalink / raw)
To: Dan Williams
Cc: Greg KH, Harold Johnson, Jonathan Cameron, linux-cxl,
Sreenivas Bagalkote, Brett Henning, Sumanesh Samanta,
linux-kernel, Davidlohr Bueso, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, linuxarm, linux-api, Lorenzo Pieralisi,
Natu, Mahesh
In-Reply-To: <662d263dd17c7_b6e0294ab@dwillia2-mobl3.amr.corp.intel.com.notmuch>
In days of yore (Sat, 27 Apr 2024), Dan Williams thus quoth:
> Greg KH wrote:
> [..]
> > So while innovating at the hardware level is fine, follow the ways that
> > everyone has done this for other specification types (USB, PCI, etc.)
> > and just allow vendor drivers to provide the information. Don't do this
> > in crazy userspace drivers which will circumvent the whole reason we
> > have standard kernel/user apis in the first place for these types of
> > things.
>
> Right, standard kernel/user apis is the requirement.
>
> The suggestion of opaque vendor passthrough tunnels, and every vendor
> ships their custom tool to do what should be common flows, is where this
> discussion went off the rails.
One aspect of this is Fabric Management (thinking CXL3 here). It is not
desirable that every vendor of CXL hardware require their own
(proprietary) fabric management software. From a user perspective, that is
absolutely horrible. Users can, and will, mix and match CXL hardware
according to their needs (or wallet), and them having to run multiple
fabric management solutions (which in worst case are conflicting with each
other) to manage things is .. suboptimal.
By all means - innovate - but do it in such a way that interoperability
and manageability is the priority. Special sauce vendor lock-in is a
surefire way to kill CXL where it stands - don't do it.
--
Kind regards,
/S
^ permalink raw reply
* Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.
From: Pavel Hofman @ 2024-04-28 11:49 UTC (permalink / raw)
To: Chris Wulff, linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, James Gruber, Lee Jones,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
Takashi Iwai
In-Reply-To: <CO1PR17MB541942196F9A2F73CF8B5B14E1152@CO1PR17MB5419.namprd17.prod.outlook.com>
On 27. 04. 24 18:27, Chris Wulff wrote:
>> From: Pavel Hofman <pavel.hofman@ivitera.com>
>>>>> + p_it_name playback input terminal name
>>>>> + p_ot_name playback output terminal name
>>>>> + p_fu_name playback function unit name
>>>>> + p_alt0_name playback alt mode 0 name
>>>>> + p_alt1_name playback alt mode 1 name
>>>>
>>>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com>
> ...
>> If the params in the upper level were to stand as defaults for the
>> altsettings (and for the existing altsetting 1 if no specific altset
>> subdir configs were given), maybe the naming xxx_alt1_xxx could become a
>> bit confusing. E.g. p_altx_name or p_alt_non0_name?
>
> I've been prototyping this a bit to see how it will work. My current configfs
> structure looks something like:
>
> (all existing properties)
> c_it_name
> c_it_ch_name
> c_fu_name
> c_ot_name
> p_it_name
> p_it_ch_name
> p_fu_name
> p_ot_name
> num_alt_modes (settable to 2..5 in my prototype)
>
> alt.0
> c_alt_name
> p_alt_name
> alt.1 (for alt.1, alt.2, etc.)
> c_alt_name
> p_alt_name
> c_ssize
> p_ssize
> (Additional properties here for other things that are settable for each alt mode,
> but the only one I've implemented in my prototype so far is sample size.)
>
Hats off to your speed, that's amazing. IMO this is a perfect config
layout, logical, extensible, easy to generate manually as well as with a
script.
> This brings up a few questions:
>
> Is a property for setting the number of alt modes preferred, or being able to
> make directories like some other things (eg, "mkdir alt.5" would add alt mode 5)?
> The former is what I started with, but I am leaning towards the latter as it is a bit
> more flexible (you could create alt modes of any index, though I'm not entirely
> sure why you'd want to.) It does involve a bit more dynamic memory allocation,
> but nothing crazy.
I am not sure the arbitrary index of alt mode would be useful (is it
even allowed in USB specs?). But I may not have understood your question
properly.
The num_alt_modes property - can the number be perhaps aquired from the
number of created directories? Or would that number of alt modes be
created automatically (all same with default values), and the properties
in alt.X dirs would subsequently only modify their respective values?
>
> And second, should the alt.x directories go back to the defaults if you remove
> and re-create them? I'm assuming it makes sense to do that, just putting it out
> there since my current prototype doesn't work that way.
IIUC just creating the alt.X directory would create the alt X mode, with
default properties from the top-level configs or with the source-code
defaults.
Thanks a lot,
Pavel.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 16:41 UTC (permalink / raw)
To: Stas Sergeev, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240426133310.1159976-1-stsp2@yandex.ru>
> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
> It is needed to perform an open operation with the creds that were in
> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
> flag. This allows the process to pre-open some dirs and switch eUID
> (and other UIDs/GIDs) to the less-privileged user, while still retaining
> the possibility to open/create files within the pre-opened directory set.
>
I’ve been contemplating this, and I want to propose a different solution.
First, the problem Stas is solving is quite narrow and doesn’t
actually need kernel support: if I want to write a user program that
sandboxes itself, I have at least three solutions already. I can make
a userns and a mountns; I can use landlock; and I can have a separate
process that brokers filesystem access using SCM_RIGHTS.
But what if I want to run a container, where the container can access
a specific host directory, and the contained application is not aware
of the exact technology being used? I recently started using
containers in anger in a production setting, and “anger” was
definitely the right word: binding part of a filesystem in is
*miserable*. Getting the DAC rules right is nasty. LSMs are worse.
Podman’s “bind,relabel” feature is IMO utterly disgusting. I think I
actually gave up on making one of my use cases work on a Fedora
system.
Here’s what I wanted to do, logically, in production: pick a host
directory, pick a host *principal* (UID, GID, label, etc), and have
the *entire container* access the directory as that principal. This is
what happens automatically if I run the whole container as a userns
with only a single UID mapped, but I don’t really want to do that for
a whole variety and of reasons.
So maybe reimagining Stas’ feature a bit can actually solve this
problem. Instead of a special dirfd, what if there was a special
subtree (in the sense of open_tree) that captures a set of creds and
does all opens inside the subtree using those creds?
This isn’t a fully formed proposal, but I *think* it should be
generally fairly safe for even an unprivileged user to clone a subtree
with a specific flag set to do this. Maybe a capability would be
needed (CAP_CAPTURE_CREDS?), but it would be nice to allow delegating
this to a daemon if a privilege is needed, and getting the API right
might be a bit tricky.
Then two different things could be done:
1. The subtree could be used unmounted or via /proc magic links. This
would be for programs that are aware of this interface.
2. The subtree could be mounted, and accessed through the mount would
use the captured creds.
(Hmm. What would a new open_tree() pointing at this special subtree do?)
With all this done, if userspace wired it up, a container user could
do something like:
—bind-capture-creds source=dest
And the contained program would access source *as the user who started
the container*, and this would just work without relabeling or
fiddling with owner uids or gids or ACLs, and it would continue to
work even if the container has multiple dynamically allocated subuids
mapped (e.g. one for “root” and one for the actual application).
Bonus points for the ability to revoke the creds in an already opened
subtree. Or even for the creds to automatically revoke themselves when
the opener exits (or maybe when a specific cred-pinning fd goes away).
(This should work for single files as well as for directories.)
New LSM hooks or extensions of existing hooks might be needed to make
LSMs comfortable with this.
What do you all think?
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 17:39 UTC (permalink / raw)
To: Andy Lutomirski, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrUL3zXAX94CpcQYwj1omwO+=-1Li+J7Bw2kpAw4d7nsyw@mail.gmail.com>
28.04.2024 19:41, Andy Lutomirski пишет:
>> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
>> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
>> It is needed to perform an open operation with the creds that were in
>> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
>> flag. This allows the process to pre-open some dirs and switch eUID
>> (and other UIDs/GIDs) to the less-privileged user, while still retaining
>> the possibility to open/create files within the pre-opened directory set.
>>
> Then two different things could be done:
>
> 1. The subtree could be used unmounted or via /proc magic links. This
> would be for programs that are aware of this interface.
>
> 2. The subtree could be mounted, and accessed through the mount would
> use the captured creds.
Doesn't this have the same problem
that was pointed to me? Namely (explaining
my impl first), that if someone puts the cred
fd to an unaware process's fd table, such
process can't fully drop its privs. He may not
want to access these dirs, but once its hacked,
the hacker will access these dirs with the
creds came from an outside.
My solution was to close such fds on
exec and disallowing SCM_RIGHTS passage.
SCM_RIGHTS can be allowed in the future,
but the receiver will need to use some
new flag to indicate that he is willing to
get such an fd. Passage via exec() can
probably never be allowed however.
If I understand your model correctly, you
put a magic sub-tree to the fs scope of some
unaware process. He may not want to access
it, but once hacked, the hacker will access
it with the creds from an outside.
And, unlike in my impl, in yours there is
probably no way to prevent that?
In short: my impl confines the hassle within
the single process. It can be extended, and
then the receiver will need to explicitly allow
adding such fds to his fd table.
But your idea seems to inherently require
2 processes, and there is probably no way
for the second process to say "ok, I allow
such sub-tree in my fs scope". And even if
he could, in my impl he can just close the
cred fd, while in yours it seems to persist.
Sorry if I misunderstood your idea.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 19:15 UTC (permalink / raw)
To: Andy Lutomirski, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <8e186307-bed2-4b5c-9bc6-bdc70171cc93@yandex.ru>
28.04.2024 20:39, stsp пишет:
> In short: my impl confines the hassle within
> the single process. It can be extended, and
> then the receiver will need to explicitly allow
> adding such fds to his fd table.
> But your idea seems to inherently require
> 2 processes, and there is probably no way
> for the second process to say "ok, I allow
> such sub-tree in my fs scope".
There is probably 1 more detail I had
to make explicit: if my model is extended
to allow SCM_RIGHTS passage by the use
of a new flag on a receiver's side, the kernel
will be able to check that the receiver
currently has the same creds as the sender.
Please note that my model is needed for the
process that does setuid() to a less-privileged
user. He would need to receive the cred fd
_before_ such setuid, he would need to use
the special flag to receive it, and the kernel
will also need to check that he has the same
creds anyway, so no escalation can ever happen.
Only that sequence of events makes the
SCM_RIGHTS passage safe, AFAICT. But if
you don't allow SCM_RIGHTS, as my current
impl does, then you are sure the process
can raise the creds only to his own ones.
Now talking about your sub-tree idea, would
it be possible to check that the process had
initially enough creds to access it w/o inheriting
anything? That part looks important to me,
i.e. the process must not access anything
that he couldn't access before dropping privs.
But I am not sure if your idea even includes
the priv drop.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 20:19 UTC (permalink / raw)
To: stsp
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <8e186307-bed2-4b5c-9bc6-bdc70171cc93@yandex.ru>
> On Apr 28, 2024, at 10:39 AM, stsp <stsp2@yandex.ru> wrote:
>
> 28.04.2024 19:41, Andy Lutomirski пишет:
>>>> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
>>> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
>>> It is needed to perform an open operation with the creds that were in
>>> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
>>> flag. This allows the process to pre-open some dirs and switch eUID
>>> (and other UIDs/GIDs) to the less-privileged user, while still retaining
>>> the possibility to open/create files within the pre-opened directory set.
>>>
>> Then two different things could be done:
>>
>> 1. The subtree could be used unmounted or via /proc magic links. This
>> would be for programs that are aware of this interface.
>>
>> 2. The subtree could be mounted, and accessed through the mount would
>> use the captured creds.
> Doesn't this have the same problem
> that was pointed to me? Namely (explaining
> my impl first), that if someone puts the cred
> fd to an unaware process's fd table, such
> process can't fully drop its privs. He may not
> want to access these dirs, but once its hacked,
> the hacker will access these dirs with the
> creds came from an outside.
This is not a real problem. If I have a writable fd for /etc/shadow or
an fd for /dev/mem, etc, then I need close them to fully drop privs.
The problem is that, in current kernels, directory fds don’t allow
access using their f_cred, and changing that means that existing
software that does not intend to create a capability will start to do
so.
> My solution was to close such fds on
> exec and disallowing SCM_RIGHTS passage.
I don't see what problem this solves.
> SCM_RIGHTS can be allowed in the future,
> but the receiver will need to use some
> new flag to indicate that he is willing to
> get such an fd. Passage via exec() can
> probably never be allowed however.
>
> If I understand your model correctly, you
> put a magic sub-tree to the fs scope of some
> unaware process.
Only if I want that process to have access!
> He may not want to access
> it, but once hacked, the hacker will access
> it with the creds from an outside.
> And, unlike in my impl, in yours there is
> probably no way to prevent that?
This is fundamental to the whole model. If I stick a FAT formatted USB
drive in the system and mount it, then any process that can find its
way to the mountpoint can write to it. And if I open a dirfd, any
process with that dirfd can write it. This is old news and isn't a
problem.
>
> In short: my impl confines the hassle within
> the single process. It can be extended, and
> then the receiver will need to explicitly allow
> adding such fds to his fd table.
> But your idea seems to inherently require
> 2 processes, and there is probably no way
> for the second process to say "ok, I allow
> such sub-tree in my fs scope".
A process could use my proposal to open_tree directory, make a whole
new mountns that is otherwise empty, switch to the mountns, mount the
directory, then change its UID and drop all privs, and still have
access to that one directory.
But even right now, a process could unshare userns and mountns, map
its uid as some nonzero number, rearrange its mountns so it only has
access to that one directory, drop all privs, and seccomp itself, and
only have access to that directory, as it still has the same UID.
Take your pick.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 21:14 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrVioWt0HUt9K1vzzuxo=Hs89AjLDUjz823s4Lwn_Y0dJw@mail.gmail.com>
28.04.2024 23:19, Andy Lutomirski пишет:
>> Doesn't this have the same problem
>> that was pointed to me? Namely (explaining
>> my impl first), that if someone puts the cred
>> fd to an unaware process's fd table, such
>> process can't fully drop its privs. He may not
>> want to access these dirs, but once its hacked,
>> the hacker will access these dirs with the
>> creds came from an outside.
> This is not a real problem. If I have a writable fd for /etc/shadow or
> an fd for /dev/mem, etc, then I need close them to fully drop privs.
But isn't that becoming a problem once
you are (maliciously) passed such fds via
exec() or SCM_RIGHTS? You may not know
about them (or about their creds), so you
won't close them. Or?
> The problem is that, in current kernels, directory fds don’t allow
> access using their f_cred, and changing that means that existing
> software that does not intend to create a capability will start to do
> so.
Which is what I am trying to do. :)
>> My solution was to close such fds on
>> exec and disallowing SCM_RIGHTS passage.
> I don't see what problem this solves.
That the process that received them,
doesn't know they have O_CRED_ALLOW
within. So it won't deduce to close them
in time.
Again, this is not the only possible solution.
The receiver can indicate its will to receive
them anyway, and the kernel can check if
such transaction is safe. But it was simpler
to just disallow, who needs to pass those?
>> SCM_RIGHTS can be allowed in the future,
>> but the receiver will need to use some
>> new flag to indicate that he is willing to
>> get such an fd. Passage via exec() can
>> probably never be allowed however.
>>
>> If I understand your model correctly, you
>> put a magic sub-tree to the fs scope of some
>> unaware process.
> Only if I want that process to have access!
Who is "I" in that context?
Can it be some malicious entity?
>> He may not want to access
>> it, but once hacked, the hacker will access
>> it with the creds from an outside.
>> And, unlike in my impl, in yours there is
>> probably no way to prevent that?
> This is fundamental to the whole model. If I stick a FAT formatted USB
> drive in the system and mount it, then any process that can find its
> way to the mountpoint can write to it. And if I open a dirfd, any
> process with that dirfd can write it. This is old news and isn't a
> problem.
But IIRC O_DIRECTORY only allows O_RDONLY.
I even re-checked now, and O_DIRECTORY|O_RDWR
gives EISDIR. So is it actually true that
whoever has dir_fd, can write to it?
>> In short: my impl confines the hassle within
>> the single process. It can be extended, and
>> then the receiver will need to explicitly allow
>> adding such fds to his fd table.
>> But your idea seems to inherently require
>> 2 processes, and there is probably no way
>> for the second process to say "ok, I allow
>> such sub-tree in my fs scope".
> A process could use my proposal to open_tree directory, make a whole
> new mountns that is otherwise empty, switch to the mountns, mount the
> directory, then change its UID and drop all privs, and still have
> access to that one directory.
Ok, if that requires actions that can't
be done after priv drop, then it can
indeed fully drop privs w/o mounting
anything, if he doesn't want such access.
Then the only security-related difference
with my approach is that mine guarantees
nothing new can be accessed, no matter
who passes what. Currently nothing can
be passed at all, but if it can - my approach
would still guarantee only the same creds
can be passed, not a new ones.
Can the same restriction be applied in
your case?
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 21:30 UTC (permalink / raw)
To: stsp
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <33bbaf98-db4f-4ea6-9f34-d1bebf06c0aa@yandex.ru>
On Sun, Apr 28, 2024 at 2:15 PM stsp <stsp2@yandex.ru> wrote:
>
> 28.04.2024 23:19, Andy Lutomirski пишет:
> >> Doesn't this have the same problem
> >> that was pointed to me? Namely (explaining
> >> my impl first), that if someone puts the cred
> >> fd to an unaware process's fd table, such
> >> process can't fully drop its privs. He may not
> >> want to access these dirs, but once its hacked,
> >> the hacker will access these dirs with the
> >> creds came from an outside.
> > This is not a real problem. If I have a writable fd for /etc/shadow or
> > an fd for /dev/mem, etc, then I need close them to fully drop privs.
>
> But isn't that becoming a problem once
> you are (maliciously) passed such fds via
> exec() or SCM_RIGHTS? You may not know
> about them (or about their creds), so you
> won't close them. Or?
Wait, who's the malicious party? Anyone who can open a directory has,
at the time they do so, permission to do so. If you send that fd to
someone via SCM_RIGHTS, all you accomplish is that they now have the
fd.
In my scenario, the malicious party attacks an *existing* program that
opens an fd for purposes that it doesn't think are dangerous. And
then it gives the fd *to the malicious program* by whatever means
(could be as simple as dropping privs then doing dlopen). Then the
malicious program does OA2_INHERIT_CREDS and gets privileges it
shouldn't have.
But if the *whole point* of opening the fd was to capture privileges
and preserve them across a privilege drop, and the program loads
malicious code after dropping privs, then that's a risk that's taken
intentionally. This is like how, if you do curl
http://whatever.com/foo.sh | bash, you are granting all kinds of
permissions to unknown code.
> >> My solution was to close such fds on
> >> exec and disallowing SCM_RIGHTS passage.
> > I don't see what problem this solves.
>
> That the process that received them,
> doesn't know they have O_CRED_ALLOW
> within. So it won't deduce to close them
> in time.
Hold on -- what exactly are you talking about? A process does
recvmsg() and doesn't trust the party at the other end. Then it
doesn't close the received fd. Then it does setuid(getuid()). Then
it does dlopen or exec of an untrusted program.
Okay, so the program now has a completely unknown fd. This is already
part of the thread model. It could be a cred-capturing fd, it could
be a device node, it could be a socket, it could be a memfd -- it
could be just about anything. How do any of your proposals or my
proposals cause an actual new problem here?
> > This is fundamental to the whole model. If I stick a FAT formatted USB
> > drive in the system and mount it, then any process that can find its
> > way to the mountpoint can write to it. And if I open a dirfd, any
> > process with that dirfd can write it. This is old news and isn't a
> > problem.
>
> But IIRC O_DIRECTORY only allows O_RDONLY.
> I even re-checked now, and O_DIRECTORY|O_RDWR
> gives EISDIR. So is it actually true that
> whoever has dir_fd, can write to it?
If the filesystem grants that UID permission to write, then it can write.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 22:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrXPgabERgWAru7PNz6A5rc6BTG9k2RRmjU71kQs4rSsPQ@mail.gmail.com>
29.04.2024 00:30, Andy Lutomirski пишет:
> On Sun, Apr 28, 2024 at 2:15 PM stsp <stsp2@yandex.ru> wrote:
>> But isn't that becoming a problem once
>> you are (maliciously) passed such fds via
>> exec() or SCM_RIGHTS? You may not know
>> about them (or about their creds), so you
>> won't close them. Or?
> Wait, who's the malicious party?
Someone who opens an fd with O_CRED_ALLOW
and passes it to an unsuspecting process. This
is at least how I understood the Christian Brauner's
point about "unsuspecting userspace".
> Anyone who can open a directory has,
> at the time they do so, permission to do so. If you send that fd to
> someone via SCM_RIGHTS, all you accomplish is that they now have the
> fd.
Normally yes.
But fd with O_CRED_ALLOW prevents the
receiver from fully dropping his privs, even
if he doesn't want to deal with it.
> In my scenario, the malicious party attacks an *existing* program that
> opens an fd for purposes that it doesn't think are dangerous. And
> then it gives the fd *to the malicious program* by whatever means
> (could be as simple as dropping privs then doing dlopen). Then the
> malicious program does OA2_INHERIT_CREDS and gets privileges it
> shouldn't have.
But what about an inverse scenario?
Malicious program passes an fd to the
"unaware" program, putting it under a
risk. That program probably never cared
about security, since it doesn't play with
privs. But suddenly it has privs, passed
out of nowhere (via exec() for example),
and someone who hacks it, takes them.
>>>> My solution was to close such fds on
>>>> exec and disallowing SCM_RIGHTS passage.
>>> I don't see what problem this solves.
>> That the process that received them,
>> doesn't know they have O_CRED_ALLOW
>> within. So it won't deduce to close them
>> in time.
> Hold on -- what exactly are you talking about? A process does
> recvmsg() and doesn't trust the party at the other end. Then it
> doesn't close the received fd. Then it does setuid(getuid()). Then
> it does dlopen or exec of an untrusted program.
>
> Okay, so the program now has a completely unknown fd. This is already
> part of the thread model. It could be a cred-capturing fd, it could
> be a device node, it could be a socket, it could be a memfd -- it
> could be just about anything. How do any of your proposals or my
> proposals cause an actual new problem here?
I am not actually sure how widely
does this spread. I.e. /dev/mem is
restricted these days, but if you can
freely pass device nodes around, then
perhaps the ability to pass an r/o dir fd
that can suddenly give creds, is probably
not something new...
But I really don't like to add to this
particular set of cases. I don't think
its safe, I just think its legacy, so while
it is done that way currently, doesn't
mean I can do the same thing and
call it "secure" just because something
like this was already possible.
Or is this actually completely safe?
Does it hurt to have O_CRED_ALLOW
non-passable?
>>> This is fundamental to the whole model. If I stick a FAT formatted USB
>>> drive in the system and mount it, then any process that can find its
>>> way to the mountpoint can write to it. And if I open a dirfd, any
>>> process with that dirfd can write it. This is old news and isn't a
>>> problem.
>> But IIRC O_DIRECTORY only allows O_RDONLY.
>> I even re-checked now, and O_DIRECTORY|O_RDWR
>> gives EISDIR. So is it actually true that
>> whoever has dir_fd, can write to it?
> If the filesystem grants that UID permission to write, then it can write.
Which to me sounds like owning an
O_DIRECTORY fd only gives you the
ability to skip the permission checks
of the outer path components, but not
the inner ones. So passing it w/o O_CRED_ALLOW
was quite safe and didn't give you any
new abilities.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-29 1:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <eae8e7e6-9c03-4c8e-ab61-cf7060d74d6d@yandex.ru>
29.04.2024 01:12, stsp пишет:
> freely pass device nodes around, then
> perhaps the ability to pass an r/o dir fd
> that can suddenly give creds, is probably
> not something new...
Actually there is probably something
new anyway. The process knows to close
all sensitive files before dropping privs.
But this may not be the case with dirs,
because dir_fd pretty much invalidates
itself when you drop privs: you'll get
EPERM from openat() if you no longer
have rights to open the file, and dir_fd
doesn't help.
Which is why someone may neglect
to close dir_fd before dropping privs,
but with O_CRED_ALLOW that would
be a mistake.
Or what about this scenario: receiver
gets this fd thinking its some useful and
harmless file fd that would be needed
even after priv drop. So he makes sure
with F_GETFL that this fd is O_RDONLY
and doesn't close it. But it appears to be
malicious O_CRED_ALLOW dir_fd, which
basically exposes many files for a write.
So I am concerned about the cases where
an fd was not closed before a priv drop,
because the process didn't realize the threat.
> But if the*whole point* of opening the fd was to capture privileges
> and preserve them across a privilege drop, and the program loads
> malicious code after dropping privs, then that's a risk that's taken
> intentionally.
If you opened an fd yourself, then yes.
You know what have you opened, and
you care to close sensitive fds before
dropping privs, or you take the risk.
But if you requested something from
another process and got some fd as
the result, the kernel doesn't guarantee
you got an fd to what you have requested.
You can get a malicious fd, which is not
"so" possible when you open an fd yourself.
So if you want to keep such an fd open,
you will probably at least make sure its
read-only, its not a device node (with fstat)
and so on.
All checks pass, and oops, O_CRED_ALLOW...
So why my patch makes O_CRED_ALLOW
non-passable? Because the receiver has
no way to see the potential harm within
such fd. So if he intended to keep an fd
open after some basic safety checks, he
will be tricked.
So while I think its a rather bad idea to
leave the received fds open after priv drop,
I don't completely exclude the possibility
that someone did that, together with a few
safety checks which would be tricked by
O_CRED_ALLOW.
^ permalink raw reply
* Re: [PATCHv3 bpf-next 5/7] selftests/bpf: Add uretprobe syscall call from user space test
From: Jiri Olsa @ 2024-04-29 7:33 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <CAEf4BzbWr9s2HiWU=7=okwH7PR8LHGFj2marmaOxKW61BWKHGg@mail.gmail.com>
On Fri, Apr 26, 2024 at 11:03:29AM -0700, Andrii Nakryiko wrote:
> On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test to verify that when called from outside of the
> > trampoline provided by kernel, the uretprobe syscall will cause
> > calling process to receive SIGILL signal and the attached bpf
> > program is no executed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../selftests/bpf/prog_tests/uprobe_syscall.c | 92 +++++++++++++++++++
> > .../selftests/bpf/progs/uprobe_syscall_call.c | 15 +++
> > 2 files changed, 107 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> >
>
> See nits below, but overall LGTM
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> [...]
>
> > @@ -219,6 +301,11 @@ static void test_uretprobe_regs_change(void)
> > {
> > test__skip();
> > }
> > +
> > +static void test_uretprobe_syscall_call(void)
> > +{
> > + test__skip();
> > +}
> > #endif
> >
> > void test_uprobe_syscall(void)
> > @@ -228,3 +315,8 @@ void test_uprobe_syscall(void)
> > if (test__start_subtest("uretprobe_regs_change"))
> > test_uretprobe_regs_change();
> > }
> > +
> > +void serial_test_uprobe_syscall_call(void)
>
> does it need to be serial? non-serial are still run sequentially
> within a process (there is no multi-threading), it's more about some
> global effects on system.
plz see below
>
> > +{
> > + test_uretprobe_syscall_call();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > new file mode 100644
> > index 000000000000..5ea03bb47198
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <string.h>
> > +
> > +struct pt_regs regs;
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("uretprobe//proc/self/exe:uretprobe_syscall_call")
> > +int uretprobe(struct pt_regs *regs)
> > +{
> > + bpf_printk("uretprobe called");
>
> debugging leftover? we probably don't want to pollute trace_pipe from test
the reason for this is to make sure the bpf program was not executed,
the test makes sure the child gets killed with SIGILL and also that
the bpf program was not executed by checking the trace_pipe and
making sure nothing was received
the trace_pipe reading is also why it's serial
jirka
>
> > + return 0;
> > +}
> > --
> > 2.44.0
> >
^ permalink raw reply
* Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From: Jiri Olsa @ 2024-04-29 7:39 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <CAEf4BzYU-y+vptqXpuALYecJJgPt+CTcbo+=Q9QXnu4vNwem+g@mail.gmail.com>
On Fri, Apr 26, 2024 at 11:06:53AM -0700, Andrii Nakryiko wrote:
> On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that adds return uprobe inside 32 bit task
> > and verify the return uprobe and attached bpf programs
> > get properly executed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/testing/selftests/bpf/.gitignore | 1 +
> > tools/testing/selftests/bpf/Makefile | 6 ++-
> > .../selftests/bpf/prog_tests/uprobe_syscall.c | 40 +++++++++++++++++++
> > .../bpf/progs/uprobe_syscall_compat.c | 13 ++++++
> > 4 files changed, 59 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> >
> > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > index f1aebabfb017..69d71223c0dd 100644
> > --- a/tools/testing/selftests/bpf/.gitignore
> > +++ b/tools/testing/selftests/bpf/.gitignore
> > @@ -45,6 +45,7 @@ test_cpp
> > /veristat
> > /sign-file
> > /uprobe_multi
> > +/uprobe_compat
> > *.ko
> > *.tmp
> > xskxceiver
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index edc73f8f5aef..d170b63eca62 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -134,7 +134,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> > xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> > xdp_features bpf_test_no_cfi.ko
> >
> > -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> > +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
>
> you need to add uprobe_compat to TRUNNER_EXTRA_FILES as well, no?
ah right
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > index 9233210a4c33..3770254d893b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > @@ -11,6 +11,7 @@
> > #include <sys/wait.h>
> > #include "uprobe_syscall.skel.h"
> > #include "uprobe_syscall_call.skel.h"
> > +#include "uprobe_syscall_compat.skel.h"
> >
> > __naked unsigned long uretprobe_regs_trigger(void)
> > {
> > @@ -291,6 +292,35 @@ static void test_uretprobe_syscall_call(void)
> > "read_trace_pipe_iter");
> > ASSERT_EQ(found, 0, "found");
> > }
> > +
> > +static void trace_pipe_compat_cb(const char *str, void *data)
> > +{
> > + if (strstr(str, "uretprobe compat") != NULL)
> > + (*(int *)data)++;
> > +}
> > +
> > +static void test_uretprobe_compat(void)
> > +{
> > + struct uprobe_syscall_compat *skel = NULL;
> > + int err, found = 0;
> > +
> > + skel = uprobe_syscall_compat__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_compat__open_and_load"))
> > + goto cleanup;
> > +
> > + err = uprobe_syscall_compat__attach(skel);
> > + if (!ASSERT_OK(err, "uprobe_syscall_compat__attach"))
> > + goto cleanup;
> > +
> > + system("./uprobe_compat");
> > +
> > + ASSERT_OK(read_trace_pipe_iter(trace_pipe_compat_cb, &found, 1000),
> > + "read_trace_pipe_iter");
>
> why so complicated? can't you just set global variable that it was called
hm, we execute separate uprobe_compat (32bit) process that triggers the bpf
program, so we can't use global variable.. using the trace_pipe was the only
thing that was easy to do
jirka
>
> > + ASSERT_EQ(found, 1, "found");
> > +
> > +cleanup:
> > + uprobe_syscall_compat__destroy(skel);
> > +}
> > #else
> > static void test_uretprobe_regs_equal(void)
> > {
> > @@ -306,6 +336,11 @@ static void test_uretprobe_syscall_call(void)
> > {
> > test__skip();
> > }
> > +
> > +static void test_uretprobe_compat(void)
> > +{
> > + test__skip();
> > +}
> > #endif
> >
> > void test_uprobe_syscall(void)
> > @@ -320,3 +355,8 @@ void serial_test_uprobe_syscall_call(void)
> > {
> > test_uretprobe_syscall_call();
> > }
> > +
> > +void serial_test_uprobe_syscall_compat(void)
>
> and then no need for serial_test?
>
> > +{
> > + test_uretprobe_compat();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > new file mode 100644
> > index 000000000000..f8adde7f08e2
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("uretprobe.multi/./uprobe_compat:main")
> > +int uretprobe_compat(struct pt_regs *ctx)
> > +{
> > + bpf_printk("uretprobe compat\n");
> > + return 0;
> > +}
> > --
> > 2.44.0
> >
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Christian Brauner @ 2024-04-29 9:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stas Sergeev, Aleksa Sarai, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrUL3zXAX94CpcQYwj1omwO+=-1Li+J7Bw2kpAw4d7nsyw@mail.gmail.com>
On Sun, Apr 28, 2024 at 09:41:20AM -0700, Andy Lutomirski wrote:
> > On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
> > This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
> > It is needed to perform an open operation with the creds that were in
> > effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
> > flag. This allows the process to pre-open some dirs and switch eUID
> > (and other UIDs/GIDs) to the less-privileged user, while still retaining
> > the possibility to open/create files within the pre-opened directory set.
> >
>
> I’ve been contemplating this, and I want to propose a different solution.
>
> First, the problem Stas is solving is quite narrow and doesn’t
> actually need kernel support: if I want to write a user program that
> sandboxes itself, I have at least three solutions already. I can make
> a userns and a mountns; I can use landlock; and I can have a separate
> process that brokers filesystem access using SCM_RIGHTS.
>
> But what if I want to run a container, where the container can access
> a specific host directory, and the contained application is not aware
> of the exact technology being used? I recently started using
> containers in anger in a production setting, and “anger” was
> definitely the right word: binding part of a filesystem in is
> *miserable*. Getting the DAC rules right is nasty. LSMs are worse.
Nowadays it's extremely simple due tue open_tree(OPEN_TREE_CLONE) and
move_mount(). I rewrote the bind-mount logic in systemd based on that
and util-linux uses that as well now.
https://brauner.io/2023/02/28/mounting-into-mount-namespaces.html
> Podman’s “bind,relabel” feature is IMO utterly disgusting. I think I
> actually gave up on making one of my use cases work on a Fedora
> system.
>
> Here’s what I wanted to do, logically, in production: pick a host
> directory, pick a host *principal* (UID, GID, label, etc), and have
> the *entire container* access the directory as that principal. This is
> what happens automatically if I run the whole container as a userns
> with only a single UID mapped, but I don’t really want to do that for
> a whole variety and of reasons.
You're describing idmapped mounts for the most part which are upstream
and are used in exactly that way by a lot of userspace.
>
> So maybe reimagining Stas’ feature a bit can actually solve this
> problem. Instead of a special dirfd, what if there was a special
> subtree (in the sense of open_tree) that captures a set of creds and
> does all opens inside the subtree using those creds?
That would mean override creds in the VFS layer when accessing a
specific subtree which is a terrible idea imho. Not just because it will
quickly become a potential dos when you do that with a lot of subtrees
it will also have complex interactions with overlayfs.
>
> This isn’t a fully formed proposal, but I *think* it should be
> generally fairly safe for even an unprivileged user to clone a subtree
> with a specific flag set to do this. Maybe a capability would be
> needed (CAP_CAPTURE_CREDS?), but it would be nice to allow delegating
> this to a daemon if a privilege is needed, and getting the API right
> might be a bit tricky.
>
> Then two different things could be done:
>
> 1. The subtree could be used unmounted or via /proc magic links. This
> would be for programs that are aware of this interface.
>
> 2. The subtree could be mounted, and accessed through the mount would
> use the captured creds.
>
> (Hmm. What would a new open_tree() pointing at this special subtree do?)
>
>
> With all this done, if userspace wired it up, a container user could
> do something like:
>
> —bind-capture-creds source=dest
>
> And the contained program would access source *as the user who started
> the container*, and this would just work without relabeling or
> fiddling with owner uids or gids or ACLs, and it would continue to
> work even if the container has multiple dynamically allocated subuids
> mapped (e.g. one for “root” and one for the actual application).
>
> Bonus points for the ability to revoke the creds in an already opened
> subtree. Or even for the creds to automatically revoke themselves when
> the opener exits (or maybe when a specific cred-pinning fd goes away).
>
> (This should work for single files as well as for directories.)
>
> New LSM hooks or extensions of existing hooks might be needed to make
> LSMs comfortable with this.
>
> What do you all think?
I think the problem you're describing is already mostly solved.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-29 12:18 UTC (permalink / raw)
To: Harold Johnson
Cc: Dan Williams, linux-cxl, Sreenivas Bagalkote, Brett Henning,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <6351024b5d6206c4e9a8cd98d1a09d43@mail.gmail.com>
On Fri, 26 Apr 2024 14:25:29 -0500
Harold Johnson <harold.johnson@broadcom.com> wrote:
> Perhaps a bit more color on a few specifics might be helpful.
>
> I think that there will always be a class of vendor specific APIs/Opcodes
> that are related to an implementation of a standard instead of the
> standard itself. I've been party to discussion on not creating CXL
> defined API/Opcodes that get into the realm of specifying an
> implementation. There are also a class of data that can be collected from
> a specific implementation that is helpful for debug, for health
> monitoring, and perhaps performance monitoring where the implementation
> matters and therefore are not easily abstracted to a standard.
Hi Harold,
Let's divert into a few specifics to give some routes to implementing these.
Some of them are extensions of things already well handled. Tweaks
and extensions in the 'spirit' of the existing spec are both places where
adding some richness to the spec is probably not too difficult and where
there may be some flexibility.
In some cases the definitions in the specification almost certainly
came after your design.
>
> A few examples:
> a) Temperature monitoring of a component or internal chip die
> temperatures. Could CXL define a standard OpCode to gather temperatures,
> yes it could; but is this really part of CXL? Then how many temperature
> elements and what does each element mean? This enters into the
> implementation and therefore is vendor specific. Unless the CXL spec
> starts to define the implementation, something along the lines of "thou
> shall have an average die temperature, rather than specific temperatures
> across a die", etc.
There is a general temperature monitoring and trip thresholds etc in the
spec via get Health Info and event logs. That covers a single value,
so not as extensive as what you refer to but it's a start. Whilst you
are right that the specification should not mandate N temperatures in
specific locations, it would be a reasonable request to allow for a
wider set of monitors with some level of description. The intent
being that generic software can discover what is there and present
that info for logging / monitoring.
Whilst the exact meaning will vary by device, some broad scoping is
easy enough (memory controller vs various FRUs perhaps) and that is useful
to providing generic users space software. hmwon has long handled this
sort of data from whole systems where very similar aspects of 'what does
this monitor actually mean' apply. Sometimes that does need device
specific mapping files - so there is precedence that may be helpful here.
>
> b) Error counters, metrics, internal counters, etc. Could CXL define a
> set of common error counters, absolutely. PCIe has done some of this.
> However, a specific implementation may have counters and error reporting
> that are meaningful only to a specific design and a specific
> implementation rather than a "least common denominator" approach of a
> standard body.
>
> c) Performance counters, metric, indicators, etc. Performance can be very
> implementation specific and tweaking performance is likely to be
> implementation specific. Yes, generic and a least common denominator
> elements could be created, but are likely to limiting in realizing the
> maximum performance of an implementation.
These two are somewhat related.
This selection falls into two broad categories.
- Opaque logging and reporting needed just for debug. There are patches
on list for Vendor Debug Log. They haven't merged yet though I think,
so good to get your input on that series. Intent of that feature is
opaque logs. There will never be a useful general tool for that stuff,
conversely no general userspace tools will use them as a result.
It's likely vendor engineer only territory.
- Counters that useful at runtime.
The CXL PMU spec is rich and flexible. In common with CPU PMUs the perf driver
allows for direct specification of events to count. The same issue with
implementation specific counters occurs in CPUs. Whilst you can keep
the meaning of events opaque, if you do you loose out on useable general
purpose software (e.g. perftool). Alternatively some of this can be
pushed into perf tool description files.
The advantage of pushing counters in to the main specification is that
they work out of the box as the CPMU driver will report those directly
(no need for perf tool to work with raw event codes).
At the moment that driver implements part of the CPMU spec. If there are
features you need (free running counters come to mind) then shout
/ patches welcome. We decided to play wait and see with the CPMU driver as
it wasn't clear if the full flexibility was needed for real devices.
>
> d) Logs, errors and debug information. In addition to spec defined
> logging of CXL topology errors, specific designs will have logs, crash
> dumps, debug data that is very specific to a implementation. There are
> likely to be cases where a product that conforms to a specification like
> CXL, may have features that don't directly have anything to do with CXL,
> but where a standards based management interface can be used to configure,
> manage, and collect data for a non-CXL feature.
There are standard interfaces defined for some of this stuff as well.
Last I checked the discussion revolved around component state dump
only being safe to use if the background command abort was supported,
as that was needed to ensure the kernel was not locked out for an indefinite
amount of time.
If you are looking at other standards being run to configure CXL devices
excellent, but those standards should be accessed using whatever the
appropriate kernel interfaces. Could you give some examples for this one?
>
> e) Innovation. I believe that innovation should be encouraged. There may
> be designs that support CXL, but that also incorporate unique and
> innovative features or functions that might service a niche market. The
> AI space is ripe for innovation and perhaps specialized features that may
> not make sense for the overall CXL specification.
Agreed - but those may need specific drivers.
>
> I think that in most cases Vendor specific opcodes are not used to
> circumvent the standards, but are used when the standards group has no
> interested in driving into the standard certain features that are clearly
> either implementation specific or are vendor specific additions that have
> a specific appeal to a select class of customer, but yet are not relevant
> to a specific standard.
>
> At the end of the day, customer want products that solve a specific
> problem. Sometimes vendor can address market segments or niches that a
> standard group has no interest in supporting. It can also take months,
> and in some cases years to reach an agreement on what standardized feature
> should look like. I also believe that there can be competitive reasons
> why there might be a group that wants to slow down a vendor's
> implementation for fear of losing market share.
Whilst I appreciate the way this can slow down adoption / kernel support
it's a path that is still worth it in the end.
As Dan and others have put it, there are other routes than the main
CXL standard. Those should allow tighter collaboration on smaller topics
to define common standards.
Thanks,
Jonathan
>
> Thanks
> Harold Johnson
>
>
> -----Original Message-----
> From: Jonathan Cameron [mailto:Jonathan.Cameron@Huawei.com]
> Sent: Friday, April 26, 2024 11:54 AM
> To: Dan Williams
> Cc: linux-cxl@vger.kernel.org; Sreenivas Bagalkote; Brett Henning; Harold
> Johnson; Sumanesh Samanta; linux-kernel@vger.kernel.org; Davidlohr Bueso;
> Dave Jiang; Alison Schofield; Vishal Verma; Ira Weiny;
> linuxarm@huawei.com; linux-api@vger.kernel.org; Lorenzo Pieralisi; Natu,
> Mahesh; gregkh@linuxfoundation.org
> Subject: Re: RFC: Restricting userspace interfaces for CXL fabric
> management
>
> On Fri, 26 Apr 2024 09:16:44 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Jonathan Cameron wrote:
> > [..]
> > > To give people an incentive to play the standards game we have to
> > > provide an alternative. Userspace libraries will provide some
> incentive
> > > to standardize if we have enough vendors (we don't today - so they
> will
> > > do their own libraries), but it is a lot easier to encourage if we
> > > exercise control over the interface.
> >
> > Yes, and I expect you and I are not far off on what can be done
> > here.
> >
> > However, lets cut to a sentiment hanging over this discussion. Referring
> > to vendor specific commands:
> >
> > "CXL spec has them for a reason and they need to be supported."
> >
> > ...that is an aggressive "vendor specific first" sentiment that
> > generates an aggressive "userspace drivers" reaction, because the best
> > way to get around community discussions about what ABI makes sense is
> > userspace drivers.
> >
> > Now, if we can step back to where this discussion started, where typical
> > Linux collaboration shines, and where I think you and I are more aligned
> > than this thread would indicate, is "vendor specific last". Lets
> > carefully consider the vendor specific commands that are candidates to
> > be de facto cross vendor semantics if not de jure standards.
> >
>
> Agreed. I'd go a little further and say I generally have much more warm
> and
> fuzzy feelings when what is a vendor defined command (today) maps to more
> or less the same bit of code for a proposed standards ECN.
>
> IP rules prevent us commenting on specific proposals, but there will be
> things we review quicker and with a lighter touch vs others where we
> ask lots of annoying questions about generality of the feature etc.
> Given the effort we are putting in on the kernel side we all want CXL
> to succeed and will do our best to encourage activities that make that
> more likely. There are other standards bodies available... which may
> make more sense for some features.
>
> Command interfaces are not a good place to compete and maintain secrecy.
> If vendors want to do that, then they don't get the pony of upstream
> support. They get to convince distros to do a custom kernel build for
> them:
> Good luck with that, some of those folk are 'blunt' in their responses to
> such requests.
>
> My proposal is we go forward with a bunch of the CXL spec defined commands
> to show the 'how' and consider specific proposals for upstream support
> of vendor defined commands on a case by case basis (so pretty much
> what you say above). Maybe after a few are done we can formalize some
> rules of thumb help vendors makes such proposals, though maybe some
> will figure out it is a better and longer term solution to do 'standards
> first development'.
>
> I think we do need to look at the safety filtering of tunneled
> commands but don't see that as a particularly tricky addition -
> for the simple non destructive commands at least.
>
> Jonathan
>
^ permalink raw reply
* Re: [PATCHv3 bpf-next 5/7] selftests/bpf: Add uretprobe syscall call from user space test
From: Andrii Nakryiko @ 2024-04-29 16:41 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <Zi9NPfII8I7nWz6O@krava>
On Mon, Apr 29, 2024 at 12:33 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Apr 26, 2024 at 11:03:29AM -0700, Andrii Nakryiko wrote:
> > On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding test to verify that when called from outside of the
> > > trampoline provided by kernel, the uretprobe syscall will cause
> > > calling process to receive SIGILL signal and the attached bpf
> > > program is no executed.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > .../selftests/bpf/prog_tests/uprobe_syscall.c | 92 +++++++++++++++++++
> > > .../selftests/bpf/progs/uprobe_syscall_call.c | 15 +++
> > > 2 files changed, 107 insertions(+)
> > > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > >
> >
> > See nits below, but overall LGTM
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > [...]
> >
> > > @@ -219,6 +301,11 @@ static void test_uretprobe_regs_change(void)
> > > {
> > > test__skip();
> > > }
> > > +
> > > +static void test_uretprobe_syscall_call(void)
> > > +{
> > > + test__skip();
> > > +}
> > > #endif
> > >
> > > void test_uprobe_syscall(void)
> > > @@ -228,3 +315,8 @@ void test_uprobe_syscall(void)
> > > if (test__start_subtest("uretprobe_regs_change"))
> > > test_uretprobe_regs_change();
> > > }
> > > +
> > > +void serial_test_uprobe_syscall_call(void)
> >
> > does it need to be serial? non-serial are still run sequentially
> > within a process (there is no multi-threading), it's more about some
> > global effects on system.
>
> plz see below
>
> >
> > > +{
> > > + test_uretprobe_syscall_call();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > > new file mode 100644
> > > index 000000000000..5ea03bb47198
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > > @@ -0,0 +1,15 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include "vmlinux.h"
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <string.h>
> > > +
> > > +struct pt_regs regs;
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +SEC("uretprobe//proc/self/exe:uretprobe_syscall_call")
> > > +int uretprobe(struct pt_regs *regs)
> > > +{
> > > + bpf_printk("uretprobe called");
> >
> > debugging leftover? we probably don't want to pollute trace_pipe from test
>
> the reason for this is to make sure the bpf program was not executed,
>
> the test makes sure the child gets killed with SIGILL and also that
> the bpf program was not executed by checking the trace_pipe and
> making sure nothing was received
>
> the trace_pipe reading is also why it's serial
you could have attached BPF program from parent process and use a
global variable (and thus eliminate all the trace_pipe system-wide
dependency), but ok, it's fine by me the way this is done
>
> jirka
>
> >
> > > + return 0;
> > > +}
> > > --
> > > 2.44.0
> > >
^ permalink raw reply
* Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From: Andrii Nakryiko @ 2024-04-29 16:44 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <Zi9OwCwluxTo-Azd@krava>
On Mon, Apr 29, 2024 at 12:39 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Apr 26, 2024 at 11:06:53AM -0700, Andrii Nakryiko wrote:
> > On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding test that adds return uprobe inside 32 bit task
> > > and verify the return uprobe and attached bpf programs
> > > get properly executed.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > tools/testing/selftests/bpf/.gitignore | 1 +
> > > tools/testing/selftests/bpf/Makefile | 6 ++-
> > > .../selftests/bpf/prog_tests/uprobe_syscall.c | 40 +++++++++++++++++++
> > > .../bpf/progs/uprobe_syscall_compat.c | 13 ++++++
> > > 4 files changed, 59 insertions(+), 1 deletion(-)
> > > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > > index f1aebabfb017..69d71223c0dd 100644
> > > --- a/tools/testing/selftests/bpf/.gitignore
> > > +++ b/tools/testing/selftests/bpf/.gitignore
> > > @@ -45,6 +45,7 @@ test_cpp
> > > /veristat
> > > /sign-file
> > > /uprobe_multi
> > > +/uprobe_compat
> > > *.ko
> > > *.tmp
> > > xskxceiver
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index edc73f8f5aef..d170b63eca62 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -134,7 +134,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> > > xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> > > xdp_features bpf_test_no_cfi.ko
> > >
> > > -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> > > +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
> >
> > you need to add uprobe_compat to TRUNNER_EXTRA_FILES as well, no?
>
> ah right
>
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > index 9233210a4c33..3770254d893b 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > > @@ -11,6 +11,7 @@
> > > #include <sys/wait.h>
> > > #include "uprobe_syscall.skel.h"
> > > #include "uprobe_syscall_call.skel.h"
> > > +#include "uprobe_syscall_compat.skel.h"
> > >
> > > __naked unsigned long uretprobe_regs_trigger(void)
> > > {
> > > @@ -291,6 +292,35 @@ static void test_uretprobe_syscall_call(void)
> > > "read_trace_pipe_iter");
> > > ASSERT_EQ(found, 0, "found");
> > > }
> > > +
> > > +static void trace_pipe_compat_cb(const char *str, void *data)
> > > +{
> > > + if (strstr(str, "uretprobe compat") != NULL)
> > > + (*(int *)data)++;
> > > +}
> > > +
> > > +static void test_uretprobe_compat(void)
> > > +{
> > > + struct uprobe_syscall_compat *skel = NULL;
> > > + int err, found = 0;
> > > +
> > > + skel = uprobe_syscall_compat__open_and_load();
> > > + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_compat__open_and_load"))
> > > + goto cleanup;
> > > +
> > > + err = uprobe_syscall_compat__attach(skel);
> > > + if (!ASSERT_OK(err, "uprobe_syscall_compat__attach"))
> > > + goto cleanup;
> > > +
> > > + system("./uprobe_compat");
> > > +
> > > + ASSERT_OK(read_trace_pipe_iter(trace_pipe_compat_cb, &found, 1000),
> > > + "read_trace_pipe_iter");
> >
> > why so complicated? can't you just set global variable that it was called
>
> hm, we execute separate uprobe_compat (32bit) process that triggers the bpf
> program, so we can't use global variable.. using the trace_pipe was the only
> thing that was easy to do
you need child process to trigger uprobe, but you could have installed
BPF program from parent process (you'd need to make child wait for
parent to be ready, with normal pipe() like we do in other places).
I think generally the less work forked child process does, the better.
All those ASSERT() failures won't produce any output in child process,
unless you run tests in verbose mode, because we haven't implemented
some form of sending all the logs back to the parent process and so
they are completely lost. But that's a separate topic.
Either way, consider using pipe() to coordinate waiting from child on
parent being ready, but otherwise do all the BPF-related heavy lifting
from parent (you can attach BPF programs to specific PID using
bpf_program__attach_uprobe() easily, it's not declarative, but simple
enough).
>
> jirka
>
> >
> > > + ASSERT_EQ(found, 1, "found");
> > > +
> > > +cleanup:
> > > + uprobe_syscall_compat__destroy(skel);
> > > +}
> > > #else
> > > static void test_uretprobe_regs_equal(void)
> > > {
> > > @@ -306,6 +336,11 @@ static void test_uretprobe_syscall_call(void)
> > > {
> > > test__skip();
> > > }
> > > +
> > > +static void test_uretprobe_compat(void)
> > > +{
> > > + test__skip();
> > > +}
> > > #endif
> > >
> > > void test_uprobe_syscall(void)
> > > @@ -320,3 +355,8 @@ void serial_test_uprobe_syscall_call(void)
> > > {
> > > test_uretprobe_syscall_call();
> > > }
> > > +
> > > +void serial_test_uprobe_syscall_compat(void)
> >
> > and then no need for serial_test?
> >
> > > +{
> > > + test_uretprobe_compat();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > > new file mode 100644
> > > index 000000000000..f8adde7f08e2
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > > @@ -0,0 +1,13 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <linux/bpf.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +SEC("uretprobe.multi/./uprobe_compat:main")
> > > +int uretprobe_compat(struct pt_regs *ctx)
> > > +{
> > > + bpf_printk("uretprobe compat\n");
> > > + return 0;
> > > +}
> > > --
> > > 2.44.0
> > >
^ permalink raw reply
* Re: [PATCH v3 2/2] fs/xattr: add *at family syscalls
From: Jan Kara @ 2024-04-30 10:09 UTC (permalink / raw)
To: cgzones
Cc: x86, linux-alpha, linux-kernel, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, audit, linux-arch, linux-api,
linux-security-module, selinux, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Michael Ellerman, Nicholas Piggin, Christophe Leroy,
Aneesh Kumar K.V, Naveen N. Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, Eric Paris,
Arnd Bergmann, Jens Axboe, Pavel Begunkov, Peter Zijlstra,
Sohil Mehta, Palmer Dabbelt, Miklos Szeredi, Nhat Pham,
Casey Schaufler, Florian Fainelli, Kees Cook, Rick Edgecombe,
Mark Rutland, io-uring
In-Reply-To: <20240426162042.191916-1-cgoettsche@seltendoof.de>
On Fri 26-04-24 18:20:14, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
>
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc/<pid>/fd/<fd> detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions and without a file
> descriptor opened with read access requiring SELinux read permission.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Pass the value of the extended attribute, its length, and for
> setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
> struct xattr_args to not exceed six syscall arguments and not
> merging the AT_* and XATTR_* flags.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The patch looks good to me. Just a few nits below:
> -static int path_setxattr(const char __user *pathname,
> +static int do_setxattrat(int dfd, const char __user *pathname, unsigned int at_flags,
Can we please stay within 80 columns (happens in multiple places in the
patch)? I don't insist but it makes things easier to read in some setups so
I prefer it.
> @@ -852,13 +908,21 @@ listxattr(struct dentry *d, char __user *list, size_t size)
> return error;
> }
>
> -static ssize_t path_listxattr(const char __user *pathname, char __user *list,
> - size_t size, unsigned int lookup_flags)
> +static ssize_t do_listxattrat(int dfd, const char __user *pathname, char __user *list,
> + size_t size, int flags)
So I like how in previous syscalls you have 'at_flags', 'lookup_flags', and
'xattr_flags'. That makes things much easier to digest. Can you please stay
with that convention here as well and call this argument 'at_flags'? Also I
think the argument ordering like "dfd, pathname, at_flags, list, size" is
more consistent with other syscalls you define.
> @@ -870,16 +934,22 @@ static ssize_t path_listxattr(const char __user *pathname, char __user *list,
> return error;
> }
>
> +SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname, char __user *, list,
> + size_t, size, int, flags)
> +{
> + return do_listxattrat(dfd, pathname, list, size, flags);
> +}
> +
Same comment as above - "flags" -> "at_flags" and reorder args please.
> @@ -917,13 +987,21 @@ removexattr(struct mnt_idmap *idmap, struct dentry *d,
> return vfs_removexattr(idmap, d, kname);
> }
>
> -static int path_removexattr(const char __user *pathname,
> - const char __user *name, unsigned int lookup_flags)
> +static int do_removexattrat(int dfd, const char __user *pathname,
> + const char __user *name, int flags)
> {
Same comment as above - "flags" -> "at_flags" and reorder args please.
> @@ -939,16 +1017,22 @@ static int path_removexattr(const char __user *pathname,
> return error;
> }
>
> +SYSCALL_DEFINE4(removexattrat, int, dfd, const char __user *, pathname,
> + const char __user *, name, int, flags)
> +{
Same comment as above - "flags" -> "at_flags" and reorder args please.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH] fs/xattr: unify *at syscalls
From: Christian Göttsche @ 2024-04-30 15:19 UTC (permalink / raw)
To: brauner
Cc: Christian Göttsche, Jan Kara, Alexander Viro, Jan Kara,
Arnd Bergmann, Thomas Gleixner, Kees Cook, Geert Uytterhoeven,
Casey Schaufler, peterz@infradead.org, Sohil Mehta,
Miklos Szeredi, Mark Rutland, linux-fsdevel, linux-kernel,
linux-api
From: Christian Göttsche <cgzones@googlemail.com>
Use the same parameter ordering for all four newly added *xattrat
syscalls:
dirfd, pathname, at_flags, ...
Also consistently use unsigned int as the type for at_flags.
Suggested-by: Jan Kara <jack@suse.com>
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
fs/xattr.c | 36 +++++++++++++++++++-----------------
include/linux/syscalls.h | 8 +++++---
2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c
index 45603e74c632..454304046d7d 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -931,17 +931,18 @@ listxattr(struct dentry *d, char __user *list, size_t size)
return error;
}
-static ssize_t do_listxattrat(int dfd, const char __user *pathname, char __user *list,
- size_t size, int flags)
+static ssize_t do_listxattrat(int dfd, const char __user *pathname,
+ unsigned int at_flags,
+ char __user *list, size_t size)
{
struct path path;
ssize_t error = 0;
int lookup_flags;
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
return -EINVAL;
- if (flags & AT_EMPTY_PATH && vfs_empty_path(dfd, pathname)) {
+ if (at_flags & AT_EMPTY_PATH && vfs_empty_path(dfd, pathname)) {
CLASS(fd, f)(dfd);
if (!f.file)
@@ -965,22 +966,23 @@ static ssize_t do_listxattrat(int dfd, const char __user *pathname, char __user
return error;
}
-SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname, char __user *, list,
- size_t, size, int, flags)
+SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname,
+ unsigned int, at_flags,
+ char __user *, list, size_t, size)
{
- return do_listxattrat(dfd, pathname, list, size, flags);
+ return do_listxattrat(dfd, pathname, at_flags, list, size);
}
SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
size_t, size)
{
- return do_listxattrat(AT_FDCWD, pathname, list, size, 0);
+ return do_listxattrat(AT_FDCWD, pathname, 0, list, size);
}
SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
size_t, size)
{
- return do_listxattrat(AT_FDCWD, pathname, list, size, AT_SYMLINK_NOFOLLOW);
+ return do_listxattrat(AT_FDCWD, pathname, AT_SYMLINK_NOFOLLOW, list, size);
}
SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
@@ -1019,17 +1021,17 @@ removexattr(struct mnt_idmap *idmap, struct dentry *d,
}
static int do_removexattrat(int dfd, const char __user *pathname,
- const char __user *name, int flags)
+ unsigned int at_flags, const char __user *name)
{
struct path path;
int error;
int lookup_flags;
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
return -EINVAL;
- lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
- if (flags & AT_EMPTY_PATH)
+ lookup_flags = (at_flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+ if (at_flags & AT_EMPTY_PATH)
lookup_flags |= LOOKUP_EMPTY;
retry:
error = user_path_at(dfd, pathname, lookup_flags, &path);
@@ -1049,21 +1051,21 @@ static int do_removexattrat(int dfd, const char __user *pathname,
}
SYSCALL_DEFINE4(removexattrat, int, dfd, const char __user *, pathname,
- const char __user *, name, int, flags)
+ unsigned int, at_flags, const char __user *, name)
{
- return do_removexattrat(dfd, pathname, name, flags);
+ return do_removexattrat(dfd, pathname, at_flags, name);
}
SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
const char __user *, name)
{
- return do_removexattrat(AT_FDCWD, pathname, name, 0);
+ return do_removexattrat(AT_FDCWD, pathname, 0, name);
}
SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
const char __user *, name)
{
- return do_removexattrat(AT_FDCWD, pathname, name, AT_SYMLINK_NOFOLLOW);
+ return do_removexattrat(AT_FDCWD, pathname, AT_SYMLINK_NOFOLLOW, name);
}
SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e06fffc48535..ca3cba698602 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -356,15 +356,17 @@ asmlinkage long sys_fgetxattr(int fd, const char __user *name,
void __user *value, size_t size);
asmlinkage long sys_listxattr(const char __user *path, char __user *list,
size_t size);
-asmlinkage long sys_listxattrat(int dfd, const char __user *path, char __user *list,
- size_t size, int flags);
+asmlinkage long sys_listxattrat(int dfd, const char __user *path,
+ unsigned int at_flags,
+ char __user *list, size_t size);
asmlinkage long sys_llistxattr(const char __user *path, char __user *list,
size_t size);
asmlinkage long sys_flistxattr(int fd, char __user *list, size_t size);
asmlinkage long sys_removexattr(const char __user *path,
const char __user *name);
asmlinkage long sys_removexattrat(int dfd, const char __user *path,
- const char __user *name, int flags);
+ unsigned int at_flags,
+ const char __user *name);
asmlinkage long sys_lremovexattr(const char __user *path,
const char __user *name);
asmlinkage long sys_fremovexattr(int fd, const char __user *name);
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 0/1] Change failover behavior for DIRECT writes in ext4/block fops
From: Jeremy Bongio @ 2024-05-01 23:15 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, linux-fsdevel, linux-api, linux-block, Jeremy Bongio
From: Jeremy Bongio <jbongio@google.com>
In kernel 6.9, for an O_DIRECT write:
xfs - Will fallback to a sync, buffered write for -ENOTBLK (for reflink CoW)
ext2/3/4 - will fallback to a sync, buffered write for short writes.
If iomap returns -ENOTBLK, write will return status of 0.
block fops - will fallback to a sync, buffered write for short writes.
zonefs - Will fallback to a sync, buffered write for -ENOTBLK.
Will return the bytes written for a short write, no fallback.
Relevant commit:
60263d5889e6d "iomap: fall back to buffered writes for invalidation failures"
In most cases, I think users would be surprised if an O_DIRECT write request
silently resulted in a buffered request.
The iomap_dio_rw() return code -ENOTBLK means page invalidation failed before
submitting the bio.
Is falling back to buffered IO for short writes or -ENOTBLK desirable in ext4
or block fops?
Jeremy Bongio (1):
Remove buffered failover for ext4 and block fops direct writes.
block/fops.c | 3 ---
fs/ext4/file.c | 27 ---------------------------
2 files changed, 30 deletions(-)
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply
* [RFC PATCH 1/1] Remove buffered failover for ext4 and block fops direct writes.
From: Jeremy Bongio @ 2024-05-01 23:15 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, linux-fsdevel, linux-api, linux-block, Jeremy Bongio
In-Reply-To: <20240501231533.3128797-1-bongiojp@gmail.com>
From: Jeremy Bongio <jbongio@google.com>
ext4 and block fops would both failover to syncronous, buffered writes if
the direct IO results in a short write where only a portion of the request
was completed.
This patch changes the behavior to simply return the number of bytes
written if the direct write is short.
---
block/fops.c | 3 ---
fs/ext4/file.c | 27 ---------------------------
2 files changed, 30 deletions(-)
diff --git a/block/fops.c b/block/fops.c
index 0cf8cf72cdfa..d32574ba9d71 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -704,9 +704,6 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (iocb->ki_flags & IOCB_DIRECT) {
ret = blkdev_direct_write(iocb, from);
- if (ret >= 0 && iov_iter_count(from))
- ret = direct_write_fallback(iocb, from, ret,
- blkdev_buffered_write(iocb, from));
} else {
ret = blkdev_buffered_write(iocb, from);
}
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 54d6ff22585c..d0760452a11f 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -595,32 +595,6 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
else
inode_unlock(inode);
- if (ret >= 0 && iov_iter_count(from)) {
- ssize_t err;
- loff_t endbyte;
-
- offset = iocb->ki_pos;
- err = ext4_buffered_write_iter(iocb, from);
- if (err < 0)
- return err;
-
- /*
- * We need to ensure that the pages within the page cache for
- * the range covered by this I/O are written to disk and
- * invalidated. This is in attempt to preserve the expected
- * direct I/O semantics in the case we fallback to buffered I/O
- * to complete off the I/O request.
- */
- ret += err;
- endbyte = offset + err - 1;
- err = filemap_write_and_wait_range(iocb->ki_filp->f_mapping,
- offset, endbyte);
- if (!err)
- invalidate_mapping_pages(iocb->ki_filp->f_mapping,
- offset >> PAGE_SHIFT,
- endbyte >> PAGE_SHIFT);
- }
-
return ret;
}
@@ -958,4 +932,3 @@ const struct inode_operations ext4_file_inode_operations = {
.fileattr_get = ext4_fileattr_get,
.fileattr_set = ext4_fileattr_set,
};
-
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: André Almeida @ 2024-05-01 23:44 UTC (permalink / raw)
To: Christian Brauner
Cc: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Paul E . McKenney, Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api, Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240426-gaumen-zweibeinig-3490b06e86c2@brauner>
Hi Christian,
Em 26/04/2024 07:26, Christian Brauner escreveu:
> On Thu, Apr 25, 2024 at 05:43:31PM -0300, André Almeida wrote:
>> Hi,
>>
>> In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
>> rseq interface to be able to implement spin locks in userspace correctly. Thomas
>> Gleixner agreed that this is something that Linux could improve, but asked for
>> an alternative proposal first: a futex operation that allows to spin a user
>> lock inside the kernel. This patchset implements a prototype of this idea for
>> further discussion.
>>
>> With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
>> be the PID of the lock owner. Then, the kernel gets the task_struct of the
>> corresponding PID, and checks if it's running. It spins until the futex
>> is awaken, the task is scheduled out or if a timeout happens. If the lock owner
>> is scheduled out at any time, then the syscall follows the normal path of
>> sleeping as usual.
>>
>> If the futex is awaken and we are spinning, we can return to userspace quickly,
>> avoid the scheduling out and in again to wake from a futex_wait(), thus
>> speeding up the wait operation.
>>
>> I didn't manage to find a good mechanism to prevent race conditions between
>> setting *futex = PID in userspace and doing find_get_task_by_vpid(PID) in kernel
>> space, giving that there's enough room for the original PID owner exit and such
>> PID to be relocated to another unrelated task in the system. I didn't performed
>
> One option would be to also allow pidfds. Starting with v6.9 they can be
> used to reference individual threads.
>
> So for the really fast case where you have multiple threads and you
> somehow may really do care about the impact of the atomic_long_inc() on
> pidfd_file->f_count during fdget() (for the single-threaded case the
> increment is elided), callers can pass the TID. But in cases where the
> inc and put aren't a performance sensitive, you can use pidfds.
>
Thank you very much for making the effort here, much appreciated :)
While I agree that pidfds would fix the PID race conditions, I will move
this interface to support TIDs instead, as noted by Florian and Peter.
With TID the race conditions are diminished I reckon?
^ permalink raw reply
* Re: [RFC PATCH 1/1] Remove buffered failover for ext4 and block fops direct writes.
From: Christoph Hellwig @ 2024-05-02 5:45 UTC (permalink / raw)
To: Jeremy Bongio
Cc: Ted Tso, linux-ext4, linux-fsdevel, linux-api, linux-block,
Jeremy Bongio
In-Reply-To: <20240501231533.3128797-2-bongiojp@gmail.com>
On Wed, May 01, 2024 at 04:15:33PM -0700, Jeremy Bongio wrote:
> From: Jeremy Bongio <jbongio@google.com>
>
> ext4 and block fops would both failover to syncronous, buffered writes if
> the direct IO results in a short write where only a portion of the request
> was completed.
>
> This patch changes the behavior to simply return the number of bytes
> written if the direct write is short.
Please don't combine ext4 and block changes in a single patch. Please
also explain why you want to change things.
AFAIK this is simply the historic behavior of the old direct I/O code
that's been around forever. I think the XFS semantics make a lot more
sense, but people might rely on this one way or another.
^ permalink raw reply
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Christian Brauner @ 2024-05-02 8:45 UTC (permalink / raw)
To: André Almeida
Cc: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Paul E . McKenney, Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api, Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <f052ff72-72c9-4b83-9285-2cd9d52e5f72@igalia.com>
On Wed, May 01, 2024 at 08:44:36PM -0300, André Almeida wrote:
> Hi Christian,
>
> Em 26/04/2024 07:26, Christian Brauner escreveu:
> > On Thu, Apr 25, 2024 at 05:43:31PM -0300, André Almeida wrote:
> > > Hi,
> > >
> > > In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
> > > rseq interface to be able to implement spin locks in userspace correctly. Thomas
> > > Gleixner agreed that this is something that Linux could improve, but asked for
> > > an alternative proposal first: a futex operation that allows to spin a user
> > > lock inside the kernel. This patchset implements a prototype of this idea for
> > > further discussion.
> > >
> > > With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
> > > be the PID of the lock owner. Then, the kernel gets the task_struct of the
> > > corresponding PID, and checks if it's running. It spins until the futex
> > > is awaken, the task is scheduled out or if a timeout happens. If the lock owner
> > > is scheduled out at any time, then the syscall follows the normal path of
> > > sleeping as usual.
> > >
> > > If the futex is awaken and we are spinning, we can return to userspace quickly,
> > > avoid the scheduling out and in again to wake from a futex_wait(), thus
> > > speeding up the wait operation.
> > >
> > > I didn't manage to find a good mechanism to prevent race conditions between
> > > setting *futex = PID in userspace and doing find_get_task_by_vpid(PID) in kernel
> > > space, giving that there's enough room for the original PID owner exit and such
> > > PID to be relocated to another unrelated task in the system. I didn't performed
> >
> > One option would be to also allow pidfds. Starting with v6.9 they can be
> > used to reference individual threads.
> >
> > So for the really fast case where you have multiple threads and you
> > somehow may really do care about the impact of the atomic_long_inc() on
> > pidfd_file->f_count during fdget() (for the single-threaded case the
> > increment is elided), callers can pass the TID. But in cases where the
> > inc and put aren't a performance sensitive, you can use pidfds.
> >
>
> Thank you very much for making the effort here, much appreciated :)
>
> While I agree that pidfds would fix the PID race conditions, I will move
> this interface to support TIDs instead, as noted by Florian and Peter. With
> TID the race conditions are diminished I reckon?
Unless I'm missing something the question here is PID (as in TGID aka
thread-group leader id gotten via getpid()) vs TID (thread specific id
gotten via gettid()). You want the thread-specific id as you want to
interact with the futex state of a specific thread not the thread-group
leader.
Aside from that TIDs are subject to the same race conditions that PIDs
are. They are allocated from the same pool (see alloc_pid()).
^ permalink raw reply
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Florian Weimer @ 2024-05-02 9:51 UTC (permalink / raw)
To: Christian Brauner
Cc: André Almeida, Mathieu Desnoyers, Peter Zijlstra,
Thomas Gleixner, linux-kernel, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, David.Laight, carlos,
Peter Oskolkov, Alexander Mikhalitsyn, Chris Kennelly,
Ingo Molnar, Darren Hart, Davidlohr Bueso, libc-alpha,
Steven Rostedt, Jonathan Corbet, Noah Goldstein,
Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240502-gezeichnet-besonderen-d277879cd669@brauner>
* Christian Brauner:
> Unless I'm missing something the question here is PID (as in TGID aka
> thread-group leader id gotten via getpid()) vs TID (thread specific id
> gotten via gettid()). You want the thread-specific id as you want to
> interact with the futex state of a specific thread not the thread-group
> leader.
>
> Aside from that TIDs are subject to the same race conditions that PIDs
> are. They are allocated from the same pool (see alloc_pid()).
For most mutex types (but not robust mutexes), it is undefined in
userspace if a thread exits while it has locked a mutex. Such a usage
condition would ensure that the race doesn't happen, I believe.
From a glibc perspective, we typically cannot use long-term file
descriptors (that are kept open across function calls) because some
applications do not expect them, or even close them behind our back.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Christian Brauner @ 2024-05-02 10:14 UTC (permalink / raw)
To: Florian Weimer
Cc: André Almeida, Mathieu Desnoyers, Peter Zijlstra,
Thomas Gleixner, linux-kernel, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, David.Laight, carlos,
Peter Oskolkov, Alexander Mikhalitsyn, Chris Kennelly,
Ingo Molnar, Darren Hart, Davidlohr Bueso, libc-alpha,
Steven Rostedt, Jonathan Corbet, Noah Goldstein,
Daniel Colascione, longman, kernel-dev
In-Reply-To: <8734r0o81v.fsf@oldenburg.str.redhat.com>
On Thu, May 02, 2024 at 11:51:56AM +0200, Florian Weimer wrote:
> * Christian Brauner:
>
> > Unless I'm missing something the question here is PID (as in TGID aka
> > thread-group leader id gotten via getpid()) vs TID (thread specific id
> > gotten via gettid()). You want the thread-specific id as you want to
> > interact with the futex state of a specific thread not the thread-group
> > leader.
> >
> > Aside from that TIDs are subject to the same race conditions that PIDs
> > are. They are allocated from the same pool (see alloc_pid()).
>
> For most mutex types (but not robust mutexes), it is undefined in
> userspace if a thread exits while it has locked a mutex. Such a usage
> condition would ensure that the race doesn't happen, I believe.
The argument is a bit shaky imho because the race not being able to
happen is predicated on no one being careless enough to exit with a
mutex held. That doesn't do anything against someone doing it on
purpose.
>
> From a glibc perspective, we typically cannot use long-term file
> descriptors (that are kept open across function calls) because some
> applications do not expect them, or even close them behind our back.
Yeah, good point. Note, I suggested it as an extension not as a
replacement for the TID. I still think it would be a useful extension in
general.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox