* Re: [PATCH v3 30/30] docs: ntsync: Add documentation for the ntsync uAPI.
From: Bagas Sanjaya @ 2024-04-12 5:52 UTC (permalink / raw)
To: Elizabeth Figura, Arnd Bergmann, Greg Kroah-Hartman,
Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Mao Zhu, Shaomin Deng,
Charles Han
In-Reply-To: <20240329000621.148791-31-zfigura@codeweavers.com>
[-- Attachment #1: Type: text/plain, Size: 17362 bytes --]
On Thu, Mar 28, 2024 at 07:06:21PM -0500, Elizabeth Figura wrote:
> diff --git a/Documentation/userspace-api/ntsync.rst b/Documentation/userspace-api/ntsync.rst
> new file mode 100644
> index 000000000000..202c2350d3af
> --- /dev/null
> +++ b/Documentation/userspace-api/ntsync.rst
> @@ -0,0 +1,399 @@
> +===================================
> +NT synchronization primitive driver
> +===================================
> +
> +This page documents the user-space API for the ntsync driver.
> +
> +ntsync is a support driver for emulation of NT synchronization
> +primitives by user-space NT emulators. It exists because implementation
> +in user-space, using existing tools, cannot match Windows performance
> +while offering accurate semantics. It is implemented entirely in
> +software, and does not drive any hardware device.
> +
> +This interface is meant as a compatibility tool only, and should not
> +be used for general synchronization. Instead use generic, versatile
> +interfaces such as futex(2) and poll(2).
> +
> +Synchronization primitives
> +==========================
> +
> +The ntsync driver exposes three types of synchronization primitives:
> +semaphores, mutexes, and events.
> +
> +A semaphore holds a single volatile 32-bit counter, and a static 32-bit
> +integer denoting the maximum value. It is considered signaled when the
> +counter is nonzero. The counter is decremented by one when a wait is
> +satisfied. Both the initial and maximum count are established when the
> +semaphore is created.
> +
> +A mutex holds a volatile 32-bit recursion count, and a volatile 32-bit
> +identifier denoting its owner. A mutex is considered signaled when its
> +owner is zero (indicating that it is not owned). The recursion count is
> +incremented when a wait is satisfied, and ownership is set to the given
> +identifier.
> +
> +A mutex also holds an internal flag denoting whether its previous owner
> +has died; such a mutex is said to be abandoned. Owner death is not
> +tracked automatically based on thread death, but rather must be
> +communicated using ``NTSYNC_IOC_MUTEX_KILL``. An abandoned mutex is
> +inherently considered unowned.
> +
> +Except for the "unowned" semantics of zero, the actual value of the
> +owner identifier is not interpreted by the ntsync driver at all. The
> +intended use is to store a thread identifier; however, the ntsync
> +driver does not actually validate that a calling thread provides
> +consistent or unique identifiers.
> +
> +An event holds a volatile boolean state denoting whether it is signaled
> +or not. There are two types of events, auto-reset and manual-reset. An
> +auto-reset event is designaled when a wait is satisfied; a manual-reset
> +event is not. The event type is specified when the event is created.
> +
> +Unless specified otherwise, all operations on an object are atomic and
> +totally ordered with respect to other operations on the same object.
> +
> +Objects are represented by files. When all file descriptors to an
> +object are closed, that object is deleted.
> +
> +Char device
> +===========
> +
> +The ntsync driver creates a single char device /dev/ntsync. Each file
> +description opened on the device represents a unique instance intended
> +to back an individual NT virtual machine. Objects created by one ntsync
> +instance may only be used with other objects created by the same
> +instance.
> +
> +ioctl reference
> +===============
> +
> +All operations on the device are done through ioctls. There are four
> +structures used in ioctl calls::
> +
> + struct ntsync_sem_args {
> + __u32 sem;
> + __u32 count;
> + __u32 max;
> + };
> +
> + struct ntsync_mutex_args {
> + __u32 mutex;
> + __u32 owner;
> + __u32 count;
> + };
> +
> + struct ntsync_event_args {
> + __u32 event;
> + __u32 signaled;
> + __u32 manual;
> + };
> +
> + struct ntsync_wait_args {
> + __u64 timeout;
> + __u64 objs;
> + __u32 count;
> + __u32 owner;
> + __u32 index;
> + __u32 alert;
> + __u32 flags;
> + __u32 pad;
> + };
> +
> +Depending on the ioctl, members of the structure may be used as input,
> +output, or not at all. All ioctls return 0 on success.
> +
> +The ioctls on the device file are as follows:
> +
> +.. c:macro:: NTSYNC_IOC_CREATE_SEM
> +
> + Create a semaphore object. Takes a pointer to struct
> + :c:type:`ntsync_sem_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``sem``
> + - On output, contains a file descriptor to the created semaphore.
> + * - ``count``
> + - Initial count of the semaphore.
> + * - ``max``
> + - Maximum count of the semaphore.
> +
> + Fails with ``EINVAL`` if ``count`` is greater than ``max``.
> +
> +.. c:macro:: NTSYNC_IOC_CREATE_MUTEX
> +
> + Create a mutex object. Takes a pointer to struct
> + :c:type:`ntsync_mutex_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``mutex``
> + - On output, contains a file descriptor to the created mutex.
> + * - ``count``
> + - Initial recursion count of the mutex.
> + * - ``owner``
> + - Initial owner of the mutex.
> +
> + If ``owner`` is nonzero and ``count`` is zero, or if ``owner`` is
> + zero and ``count`` is nonzero, the function fails with ``EINVAL``.
> +
> +.. c:macro:: NTSYNC_IOC_CREATE_EVENT
> +
> + Create an event object. Takes a pointer to struct
> + :c:type:`ntsync_event_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``event``
> + - On output, contains a file descriptor to the created event.
> + * - ``signaled``
> + - If nonzero, the event is initially signaled, otherwise
> + nonsignaled.
> + * - ``manual``
> + - If nonzero, the event is a manual-reset event, otherwise
> + auto-reset.
> +
> +The ioctls on the individual objects are as follows:
> +
> +.. c:macro:: NTSYNC_IOC_SEM_POST
> +
> + Post to a semaphore object. Takes a pointer to a 32-bit integer,
> + which on input holds the count to be added to the semaphore, and on
> + output contains its previous count.
> +
> + If adding to the semaphore's current count would raise the latter
> + past the semaphore's maximum count, the ioctl fails with
> + ``EOVERFLOW`` and the semaphore is not affected. If raising the
> + semaphore's count causes it to become signaled, eligible threads
> + waiting on this semaphore will be woken and the semaphore's count
> + decremented appropriately.
> +
> +.. c:macro:: NTSYNC_IOC_MUTEX_UNLOCK
> +
> + Release a mutex object. Takes a pointer to struct
> + :c:type:`ntsync_mutex_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``mutex``
> + - Ignored.
> + * - ``owner``
> + - Specifies the owner trying to release this mutex.
> + * - ``count``
> + - On output, contains the previous recursion count.
> +
> + If ``owner`` is zero, the ioctl fails with ``EINVAL``. If ``owner``
> + is not the current owner of the mutex, the ioctl fails with
> + ``EPERM``.
> +
> + The mutex's count will be decremented by one. If decrementing the
> + mutex's count causes it to become zero, the mutex is marked as
> + unowned and signaled, and eligible threads waiting on it will be
> + woken as appropriate.
> +
> +.. c:macro:: NTSYNC_IOC_SET_EVENT
> +
> + Signal an event object. Takes a pointer to a 32-bit integer, which on
> + output contains the previous state of the event.
> +
> + Eligible threads will be woken, and auto-reset events will be
> + designaled appropriately.
> +
> +.. c:macro:: NTSYNC_IOC_RESET_EVENT
> +
> + Designal an event object. Takes a pointer to a 32-bit integer, which
> + on output contains the previous state of the event.
> +
> +.. c:macro:: NTSYNC_IOC_PULSE_EVENT
> +
> + Wake threads waiting on an event object while leaving it in an
> + unsignaled state. Takes a pointer to a 32-bit integer, which on
> + output contains the previous state of the event.
> +
> + A pulse operation can be thought of as a set followed by a reset,
> + performed as a single atomic operation. If two threads are waiting on
> + an auto-reset event which is pulsed, only one will be woken. If two
> + threads are waiting a manual-reset event which is pulsed, both will
> + be woken. However, in both cases, the event will be unsignaled
> + afterwards, and a simultaneous read operation will always report the
> + event as unsignaled.
> +
> +.. c:macro:: NTSYNC_IOC_READ_SEM
> +
> + Read the current state of a semaphore object. Takes a pointer to
> + struct :c:type:`ntsync_sem_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``sem``
> + - Ignored.
> + * - ``count``
> + - On output, contains the current count of the semaphore.
> + * - ``max``
> + - On output, contains the maximum count of the semaphore.
> +
> +.. c:macro:: NTSYNC_IOC_READ_MUTEX
> +
> + Read the current state of a mutex object. Takes a pointer to struct
> + :c:type:`ntsync_mutex_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``mutex``
> + - Ignored.
> + * - ``owner``
> + - On output, contains the current owner of the mutex, or zero
> + if the mutex is not currently owned.
> + * - ``count``
> + - On output, contains the current recursion count of the mutex.
> +
> + If the mutex is marked as abandoned, the function fails with
> + ``EOWNERDEAD``. In this case, ``count`` and ``owner`` are set to
> + zero.
> +
> +.. c:macro:: NTSYNC_IOC_READ_EVENT
> +
> + Read the current state of an event object. Takes a pointer to struct
> + :c:type:`ntsync_event_args`, which is used as follows:
> +
> + .. list-table::
> +
> + * - ``event``
> + - Ignored.
> + * - ``signaled``
> + - On output, contains the current state of the event.
> + * - ``manual``
> + - On output, contains 1 if the event is a manual-reset event,
> + and 0 otherwise.
> +
> +.. c:macro:: NTSYNC_IOC_KILL_OWNER
> +
> + Mark a mutex as unowned and abandoned if it is owned by the given
> + owner. Takes an input-only pointer to a 32-bit integer denoting the
> + owner. If the owner is zero, the ioctl fails with ``EINVAL``. If the
> + owner does not own the mutex, the function fails with ``EPERM``.
> +
> + Eligible threads waiting on the mutex will be woken as appropriate
> + (and such waits will fail with ``EOWNERDEAD``, as described below).
> +
> +.. c:macro:: NTSYNC_IOC_WAIT_ANY
> +
> + Poll on any of a list of objects, atomically acquiring at most one.
> + Takes a pointer to struct :c:type:`ntsync_wait_args`, which is
> + used as follows:
> +
> + .. list-table::
> +
> + * - ``timeout``
> + - Absolute timeout in nanoseconds. If ``NTSYNC_WAIT_REALTIME``
> + is set, the timeout is measured against the REALTIME clock;
> + otherwise it is measured against the MONOTONIC clock. If the
> + timeout is equal to or earlier than the current time, the
> + function returns immediately without sleeping. If ``timeout``
> + is U64_MAX, the function will sleep until an object is
> + signaled, and will not fail with ``ETIMEDOUT``.
> + * - ``objs``
> + - Pointer to an array of ``count`` file descriptors
> + (specified as an integer so that the structure has the same
> + size regardless of architecture). If any object is
> + invalid, the function fails with ``EINVAL``.
> + * - ``count``
> + - Number of objects specified in the ``objs`` array.
> + If greater than ``NTSYNC_MAX_WAIT_COUNT``, the function fails
> + with ``EINVAL``.
> + * - ``owner``
> + - Mutex owner identifier. If any object in ``objs`` is a mutex,
> + the ioctl will attempt to acquire that mutex on behalf of
> + ``owner``. If ``owner`` is zero, the ioctl fails with
> + ``EINVAL``.
> + * - ``index``
> + - On success, contains the index (into ``objs``) of the object
> + which was signaled. If ``alert`` was signaled instead,
> + this contains ``count``.
> + * - ``alert``
> + - Optional event object file descriptor. If nonzero, this
> + specifies an "alert" event object which, if signaled, will
> + terminate the wait. If nonzero, the identifier must point to a
> + valid event.
> + * - ``flags``
> + - Zero or more flags. Currently the only flag is
> + ``NTSYNC_WAIT_REALTIME``, which causes the timeout to be
> + measured against the REALTIME clock instead of MONOTONIC.
> + * - ``pad``
> + - Unused, must be set to zero.
> +
> + This function attempts to acquire one of the given objects. If unable
> + to do so, it sleeps until an object becomes signaled, subsequently
> + acquiring it, or the timeout expires. In the latter case the ioctl
> + fails with ``ETIMEDOUT``. The function only acquires one object, even
> + if multiple objects are signaled.
> +
> + A semaphore is considered to be signaled if its count is nonzero, and
> + is acquired by decrementing its count by one. A mutex is considered
> + to be signaled if it is unowned or if its owner matches the ``owner``
> + argument, and is acquired by incrementing its recursion count by one
> + and setting its owner to the ``owner`` argument. An auto-reset event
> + is acquired by designaling it; a manual-reset event is not affected
> + by acquisition.
> +
> + Acquisition is atomic and totally ordered with respect to other
> + operations on the same object. If two wait operations (with different
> + ``owner`` identifiers) are queued on the same mutex, only one is
> + signaled. If two wait operations are queued on the same semaphore,
> + and a value of one is posted to it, only one is signaled. The order
> + in which threads are signaled is not specified.
> +
> + If an abandoned mutex is acquired, the ioctl fails with
> + ``EOWNERDEAD``. Although this is a failure return, the function may
> + otherwise be considered successful. The mutex is marked as owned by
> + the given owner (with a recursion count of 1) and as no longer
> + abandoned, and ``index`` is still set to the index of the mutex.
> +
> + The ``alert`` argument is an "extra" event which can terminate the
> + wait, independently of all other objects. If members of ``objs`` and
> + ``alert`` are both simultaneously signaled, a member of ``objs`` will
> + always be given priority and acquired first.
> +
> + It is valid to pass the same object more than once, including by
> + passing the same event in the ``objs`` array and in ``alert``. If a
> + wakeup occurs due to that object being signaled, ``index`` is set to
> + the lowest index corresponding to that object.
> +
> + The function may fail with ``EINTR`` if a signal is received.
> +
> +.. c:macro:: NTSYNC_IOC_WAIT_ALL
> +
> + Poll on a list of objects, atomically acquiring all of them. Takes a
> + pointer to struct :c:type:`ntsync_wait_args`, which is used
> + identically to ``NTSYNC_IOC_WAIT_ANY``, except that ``index`` is
> + always filled with zero on success if not woken via alert.
> +
> + This function attempts to simultaneously acquire all of the given
> + objects. If unable to do so, it sleeps until all objects become
> + simultaneously signaled, subsequently acquiring them, or the timeout
> + expires. In the latter case the ioctl fails with ``ETIMEDOUT`` and no
> + objects are modified.
> +
> + Objects may become signaled and subsequently designaled (through
> + acquisition by other threads) while this thread is sleeping. Only
> + once all objects are simultaneously signaled does the ioctl acquire
> + them and return. The entire acquisition is atomic and totally ordered
> + with respect to other operations on any of the given objects.
> +
> + If an abandoned mutex is acquired, the ioctl fails with
> + ``EOWNERDEAD``. Similarly to ``NTSYNC_IOC_WAIT_ANY``, all objects are
> + nevertheless marked as acquired. Note that if multiple mutex objects
> + are specified, there is no way to know which were marked as
> + abandoned.
> +
> + As with "any" waits, the ``alert`` argument is an "extra" event which
> + can terminate the wait. Critically, however, an "all" wait will
> + succeed if all members in ``objs`` are signaled, *or* if ``alert`` is
> + signaled. In the latter case ``index`` will be set to ``count``. As
> + with "any" waits, if both conditions are filled, the former takes
> + priority, and objects in ``objs`` will be acquired.
> +
> + Unlike ``NTSYNC_IOC_WAIT_ANY``, it is not valid to pass the same
> + object more than once, nor is it valid to pass the same object in
> + ``objs`` and in ``alert``. If this is attempted, the function fails
> + with ``EINVAL``.
The doc LGTM, thanks!
Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 04/30] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-04-12 0:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Jonathan Corbet, Shuah Khan, linux-kernel,
linux-api, wine-devel, André Almeida, Wolfram Sang,
Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski, linux-doc,
linux-kselftest, Randy Dunlap
In-Reply-To: <2024041111-handsaw-scruffy-27f3@gregkh>
On Thursday, 11 April 2024 08:34:23 CDT Greg Kroah-Hartman wrote:
> On Thu, Mar 28, 2024 at 07:05:55PM -0500, Elizabeth Figura wrote:
> > This corresponds to part of the functionality of the NT syscall
> > NtWaitForMultipleObjects(). Specifically, it implements the behaviour where
> > the third argument (wait_any) is TRUE, and it does not handle alertable waits.
> > Those features have been split out into separate patches to ease review.
> >
> > NTSYNC_IOC_WAIT_ANY is a vectored wait function similar to poll(). Unlike
> > poll(), it "consumes" objects when they are signaled. For semaphores, this means
> > decreasing one from the internal counter. At most one object can be consumed by
> > this function.
> >
> > Up to 64 objects can be waited on at once. As soon as one is signaled, the
> > object with the lowest index is consumed, and that index is returned via the
> > "index" field.
>
> So it's kind of like our internal locks already? Or futex?
Striking the right balance of explaining the problem space without
inundating the reader with information has been tricky; I'll do my best
to try to explain here.
The primitives include mutexes and semaphores, like our internal locks.
I don't really want to compare them to futexes because futexes don't
have internal state.
However NT's primitives are *way* more complicated. The big part of it
is they consume state in a way that usual wait functions don't, and as
if that weren't enough, you can do operations with them like
wait-for-all (wait for all objects to be simultaneously signaled and
atomically consume them) or pulse (signal an object without changing
its state). None of this can be expressed with poll or futex.
You can't even express those operations with wait_event() etc. We
really need to replace the entire wait queue and use schedule() +
wake_up_process() directly. ntsync_q is the wait queue struct in this.
They're also really ugly things to do; they only exist for
compatibility reasons, and retrofitting support into anything would
complicate and slow down hot paths.
> > A timeout is supported. The timeout is passed as a u64 nanosecond value, which
> > represents absolute time measured against either the MONOTONIC or REALTIME clock
> > (controlled by the flags argument). If U64_MAX is passed, the ioctl waits
> > indefinitely.
> >
> > This ioctl validates that all objects belong to the relevant device. This is not
> > necessary for any technical reason related to NTSYNC_IOC_WAIT_ANY, but will be
> > necessary for NTSYNC_IOC_WAIT_ALL introduced in the following patch.
> >
> > Two u32s of padding are left in the ntsync_wait_args structure; one will be used
> > by a patch later in the series (which is split out to ease review).
> >
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> > ---
> > drivers/misc/ntsync.c | 250 ++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/ntsync.h | 16 +++
> > 2 files changed, 266 insertions(+)
> >
> > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> > index 3c2f743c58b0..c6f84a5fc8c0 100644
> > --- a/drivers/misc/ntsync.c
> > +++ b/drivers/misc/ntsync.c
> > @@ -6,11 +6,16 @@
> > */
> >
> > #include <linux/anon_inodes.h>
> > +#include <linux/atomic.h>
> > #include <linux/file.h>
> > #include <linux/fs.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/ktime.h>
> > #include <linux/miscdevice.h>
> > #include <linux/module.h>
> > #include <linux/overflow.h>
> > +#include <linux/sched.h>
> > +#include <linux/sched/signal.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > #include <uapi/linux/ntsync.h>
> > @@ -30,6 +35,8 @@ enum ntsync_type {
> > *
> > * Both rely on struct file for reference counting. Individual
> > * ntsync_obj objects take a reference to the device when created.
> > + * Wait operations take a reference to each object being waited on for
> > + * the duration of the wait.
> > */
> >
> > struct ntsync_obj {
> > @@ -47,12 +54,56 @@ struct ntsync_obj {
> > __u32 max;
> > } sem;
> > } u;
> > +
> > + struct list_head any_waiters;
> > +};
> > +
> > +struct ntsync_q_entry {
> > + struct list_head node;
> > + struct ntsync_q *q;
> > + struct ntsync_obj *obj;
> > + __u32 index;
> > +};
> > +
> > +struct ntsync_q {
> > + struct task_struct *task;
> > + __u32 owner;
> > +
> > + /*
> > + * Protected via atomic_try_cmpxchg(). Only the thread that wins the
> > + * compare-and-swap may actually change object states and wake this
> > + * task.
> > + */
> > + atomic_t signaled;
>
> This feels odd, why are you duplicating a normal lock functionality
> here?
ntsync_q represents a single waiter (like struct wait_queue_entry).
In short, waiting is a destructive operation; it changes the state of
the primitives waited on. If a waiter is woken successfully then it
must have consumed the state of exactly one object.
Therefore, if task A is waiting on two primitives X and Y, and those
primitives are respectively woken at the same time by tasks B and C, we
need a way to ensure that B and C don't both wake A and consume the
state of X and Y. Only one of them should win.
We could do that with a lock on the ntsync_q struct, but having a
single variable with atomic-test-and-set achieves the same thing while
being lock-free.
> > +
> > + __u32 count;
> > + struct ntsync_q_entry entries[];
> > };
> >
> > struct ntsync_device {
> > struct file *file;
> > };
> >
> > +static void try_wake_any_sem(struct ntsync_obj *sem)
> > +{
> > + struct ntsync_q_entry *entry;
> > +
> > + lockdep_assert_held(&sem->lock);
> > +
> > + list_for_each_entry(entry, &sem->any_waiters, node) {
> > + struct ntsync_q *q = entry->q;
> > + int signaled = -1;
> > +
> > + if (!sem->u.sem.count)
> > + break;
> > +
> > + if (atomic_try_cmpxchg(&q->signaled, &signaled, entry->index)) {
> > + sem->u.sem.count--;
> > + wake_up_process(q->task);
> > + }
>
> You are waking up _all_ "locks" that with the atomic_try_cmpxchg() call,
> right? Not just the "first".
>
> Or am I confused?
This is looping over all tasks trying to lock / acquire "sem", and
waking them (assuming something else didn't wake them first) while
decrementing "sem" state accordingly.
> > + }
> > +}
> > +
> > /*
> > * Actually change the semaphore state, returning -EOVERFLOW if it is made
> > * invalid.
> > @@ -88,6 +139,8 @@ static int ntsync_sem_post(struct ntsync_obj *sem, void __user *argp)
> >
> > prev_count = sem->u.sem.count;
> > ret = post_sem_state(sem, args);
> > + if (!ret)
> > + try_wake_any_sem(sem);
> >
> > spin_unlock(&sem->lock);
> >
> > @@ -141,6 +194,7 @@ static struct ntsync_obj *ntsync_alloc_obj(struct ntsync_device *dev,
> > obj->dev = dev;
> > get_file(dev->file);
> > spin_lock_init(&obj->lock);
> > + INIT_LIST_HEAD(&obj->any_waiters);
> >
> > return obj;
> > }
> > @@ -191,6 +245,200 @@ static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
> > return put_user(fd, &user_args->sem);
> > }
> >
> > +static struct ntsync_obj *get_obj(struct ntsync_device *dev, int fd)
> > +{
> > + struct file *file = fget(fd);
> > + struct ntsync_obj *obj;
> > +
> > + if (!file)
> > + return NULL;
> > +
> > + if (file->f_op != &ntsync_obj_fops) {
> > + fput(file);
> > + return NULL;
> > + }
> > +
> > + obj = file->private_data;
> > + if (obj->dev != dev) {
> > + fput(file);
> > + return NULL;
> > + }
> > +
> > + return obj;
> > +}
> > +
> > +static void put_obj(struct ntsync_obj *obj)
> > +{
> > + fput(obj->file);
> > +}
> > +
> > +static int ntsync_schedule(const struct ntsync_q *q, const struct ntsync_wait_args *args)
> > +{
> > + ktime_t timeout = ns_to_ktime(args->timeout);
> > + clockid_t clock = CLOCK_MONOTONIC;
> > + ktime_t *timeout_ptr;
> > + int ret = 0;
> > +
> > + timeout_ptr = (args->timeout == U64_MAX ? NULL : &timeout);
> > +
> > + if (args->flags & NTSYNC_WAIT_REALTIME)
> > + clock = CLOCK_REALTIME;
> > +
> > + do {
> > + if (signal_pending(current)) {
> > + ret = -ERESTARTSYS;
> > + break;
> > + }
> > +
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + if (atomic_read(&q->signaled) != -1) {
> > + ret = 0;
> > + break;
>
> What happens if the value changes right after you read it?
The corresponding wake code flips signaled and then does
wake_up_process(), so schedule() returns immediately (and we see
q->signaled set and exit the loop.)
> Rolling your own lock is tricky, and needs review from the locking
> maintainers. And probably some more documentation as to what is
> happening and why our normal types of locks can't be used here?
Definitely. (Unfortunately this hasn't gotten attention from any locking
maintainer yet since your last call for review; not sure if there's
anything I can do there.)
Hopefully my comment at the top of this mail explains why we're rolling
our own everything, but if not please let me know and I can try to
explain more clearly.
--Zeb
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Mahesh Bandewar (महेश बंडेवार) @ 2024-04-11 16:33 UTC (permalink / raw)
To: Sagi Maimon
Cc: Thomas Gleixner, richardcochran, luto, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland,
mszeredi, casey, reibax, davem, brauner, linux-kernel, linux-api,
linux-arch, netdev
In-Reply-To: <CAMuE1bFkmj70DO66PfvBPjM1d_JDEwTkOyz6o6wO_C0uyJ_0zw@mail.gmail.com>
On Thu, Apr 11, 2024 at 12:11 AM Sagi Maimon <maimon.sagi@gmail.com> wrote:
>
> Hi Mahesh
> What is the status of your patch?
> if your patch is upstreamed , then it will have all I need.
> But, If not , I will upstream my patch.
> BR,
>
Hi Sagi,
If you want to pursue the syscall option, then those are tangential
and please go ahead. (I cannot stop you!)
I'm interested in getting the "tight sandwich timestamps" that
gettimex64() ioctl offers and I would want enhancements to
gettimex64() done the way it was discussed in the later half of this
thread. If you want to sign-up for that please let me know.
thanks,
--mahesh..
> On Thu, Apr 11, 2024 at 5:56 AM Mahesh Bandewar (महेश बंडेवार)
> <maheshb@google.com> wrote:
> >
> > On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> > > > On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > The modification that you have proposed (in a couple of posts back)
> > > > would work but it's still not ideal since the pre/post ts are not
> > > > close enough as they are currently (properly implemented!)
> > > > gettimex64() would have. The only way to do that would be to have
> > > > another ioctl as I have proposed which is a superset of current
> > > > gettimex64 and pre-post collection is the closest possible.
> > >
> > > Errm. What I posted as sketch _is_ using gettimex64() with the extra
> > > twist of the flag vs. a clockid (which is an implementation detail) and
> > > the difference that I carry the information in ptp_system_timestamp
> > > instead of needing a new argument clockid to all existing callbacks
> > > because the modification to ptp_read_prets() and postts() will just be
> > > sufficient, no?
> > >
> > OK, that makes sense.
> >
> > > For the case where the driver does not provide gettimex64() then the
> > > extension of the original offset ioctl is still providing a better
> > > mechanism than the proposed syscall.
> > >
> > > I also clearly said that all drivers should be converted over to
> > > gettimex64().
> > >
> > I agree. Honestly that should have been mandatory and
> > ptp_register_clock() should fail otherwise! Probably should have been
> > part of gettimex64 implementation :(
> >
> > I don't think we can do anything other than just hoping all driver
> > implementations include gettimex64 implementation.
> >
> > > > Having said that, the 'flag' modification proposal is a good backup
> > > > for the drivers that don't have good implementation (close enough but
> > > > not ideal). Also, you don't need a new ioctl-op. So if we really want
> > > > precision, I believe, we need a new ioctl op (with supporting
> > > > implementation similar to the mlx4 code above). but we want to save
> > > > the new ioctl-op and have less precision then proposed modification
> > > > would work fine.
> > >
> > > I disagree. The existing gettimex64() is good enough if the driver
> > > implements it correctly today. If not then those drivers need to be
> > > fixed independent of this.
> > >
> > > So assumed that a driver does:
> > >
> > > gettimex64()
> > > ptp_prets(sts);
> > > read_clock();
> > > ptp_postts(sts);
> > >
> > > today then having:
> > >
> > > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > > ktime_get_raw_ts64(&sts->pre_ts);
> > > else
> > > ktime_get_real_ts64(&sts->pre_ts);
> > > }
> > > }
> > >
> > > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > > ktime_get_raw_ts64(&sts->post_ts);
> > > else
> > > ktime_get_real_ts64(&sts->post_ts);
> > > }
> > > }
> > >
> > > or
> > >
> > > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > switch (sts->clockid) {
> > > case CLOCK_MONOTONIC_RAW:
> > > time_get_raw_ts64(&sts->pre_ts);
> > > break;
> > > case CLOCK_REALTIME:
> > > ktime_get_real_ts64(&sts->pre_ts);
> > > break;
> > > }
> > > }
> > > }
> > >
> > > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > > {
> > > if (sts) {
> > > switch (sts->clockid) {
> > > case CLOCK_MONOTONIC_RAW:
> > > time_get_raw_ts64(&sts->post_ts);
> > > break;
> > > case CLOCK_REALTIME:
> > > ktime_get_real_ts64(&sts->post_ts);
> > > break;
> > > }
> > > }
> > > }
> > >
> > > is doing the exact same thing as your proposal but without touching any
> > > driver which implements gettimex64() correctly at all.
> > >
> > I see. Yes, this makes sense.
> >
> > > While your proposal requires to touch every single driver for no reason,
> > > no?
> > >
> > > It is just an implementation detail whether you use a flag or a
> > > clockid. You can carry the clockid for the clocks which actually can be
> > > read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
> > >
> > > struct ptp_sys_offset_extended {
> > > unsigned int n_samples; /* Desired number of measurements. */
> > > clockid_t clockid;
> > > unsigned int rsv[2]; /* Reserved for future use. */
> > > };
> > >
> > > and in the IOCTL:
> > >
> > > if (extoff->clockid != CLOCK_MONOTONIC_RAW)
> > > return -EINVAL;
> > >
> > > sts.clockid = extoff->clockid;
> > >
> > > and it all just works, no?
> > >
> > Yes, this should work. However, I didn't check if struct
> > ptp_system_timestamp is used in some other context.
> >
> > > I have no problem to decide that PTP_SYS_OFFSET will not get this
> > > treatment and the drivers have to be converted over to
> > > PTP_SYS_OFFSET_EXTENDED.
> > >
> > > But adding yet another callback just to carry a clockid as argument is a
> > > more than pointless exercise as I demonstrated.
> > >
> > Agreed. As I said, I thought we cannot change the gettimex64() without
> > breaking the compatibility but the fact that CLOCK_REALTIME is "0"
> > works well for the backward compatibility case.
> >
> > I can spin up an updated patch/series that updates gettimex64
> > implementation instead of adding a new ioctl-op If you all agree.
> >
> > thanks,
> > --mahesh..
> >
> > > Thanks,
> > >
> > > tglx
^ permalink raw reply
* Re: [PATCH v3 03/30] ntsync: Introduce NTSYNC_IOC_SEM_POST.
From: Greg Kroah-Hartman @ 2024-04-11 13:35 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, Jonathan Corbet, Shuah Khan, linux-kernel,
linux-api, wine-devel, André Almeida, Wolfram Sang,
Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski, linux-doc,
linux-kselftest, Randy Dunlap
In-Reply-To: <20240329000621.148791-4-zfigura@codeweavers.com>
On Thu, Mar 28, 2024 at 07:05:54PM -0500, Elizabeth Figura wrote:
> This corresponds to the NT syscall NtReleaseSemaphore().
>
> This increases the semaphore's internal counter by the given value, and returns
> the previous value. If the counter would overflow the defined maximum, the
> function instead fails and returns -EOVERFLOW.
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> ---
> drivers/misc/ntsync.c | 72 +++++++++++++++++++++++++++++++++++--
> include/uapi/linux/ntsync.h | 2 ++
> 2 files changed, 71 insertions(+), 3 deletions(-)
As the first 3 patches here looked good to me (see my comments on patch
4), I took them to save you the trouble of constantly refreshing them.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 04/30] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Greg Kroah-Hartman @ 2024-04-11 13:34 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, Jonathan Corbet, Shuah Khan, linux-kernel,
linux-api, wine-devel, André Almeida, Wolfram Sang,
Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski, linux-doc,
linux-kselftest, Randy Dunlap
In-Reply-To: <20240329000621.148791-5-zfigura@codeweavers.com>
On Thu, Mar 28, 2024 at 07:05:55PM -0500, Elizabeth Figura wrote:
> This corresponds to part of the functionality of the NT syscall
> NtWaitForMultipleObjects(). Specifically, it implements the behaviour where
> the third argument (wait_any) is TRUE, and it does not handle alertable waits.
> Those features have been split out into separate patches to ease review.
>
> NTSYNC_IOC_WAIT_ANY is a vectored wait function similar to poll(). Unlike
> poll(), it "consumes" objects when they are signaled. For semaphores, this means
> decreasing one from the internal counter. At most one object can be consumed by
> this function.
>
> Up to 64 objects can be waited on at once. As soon as one is signaled, the
> object with the lowest index is consumed, and that index is returned via the
> "index" field.
So it's kind of like our internal locks already? Or futex?
>
> A timeout is supported. The timeout is passed as a u64 nanosecond value, which
> represents absolute time measured against either the MONOTONIC or REALTIME clock
> (controlled by the flags argument). If U64_MAX is passed, the ioctl waits
> indefinitely.
>
> This ioctl validates that all objects belong to the relevant device. This is not
> necessary for any technical reason related to NTSYNC_IOC_WAIT_ANY, but will be
> necessary for NTSYNC_IOC_WAIT_ALL introduced in the following patch.
>
> Two u32s of padding are left in the ntsync_wait_args structure; one will be used
> by a patch later in the series (which is split out to ease review).
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> ---
> drivers/misc/ntsync.c | 250 ++++++++++++++++++++++++++++++++++++
> include/uapi/linux/ntsync.h | 16 +++
> 2 files changed, 266 insertions(+)
>
> diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> index 3c2f743c58b0..c6f84a5fc8c0 100644
> --- a/drivers/misc/ntsync.c
> +++ b/drivers/misc/ntsync.c
> @@ -6,11 +6,16 @@
> */
>
> #include <linux/anon_inodes.h>
> +#include <linux/atomic.h>
> #include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/hrtimer.h>
> +#include <linux/ktime.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> #include <linux/overflow.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <uapi/linux/ntsync.h>
> @@ -30,6 +35,8 @@ enum ntsync_type {
> *
> * Both rely on struct file for reference counting. Individual
> * ntsync_obj objects take a reference to the device when created.
> + * Wait operations take a reference to each object being waited on for
> + * the duration of the wait.
> */
>
> struct ntsync_obj {
> @@ -47,12 +54,56 @@ struct ntsync_obj {
> __u32 max;
> } sem;
> } u;
> +
> + struct list_head any_waiters;
> +};
> +
> +struct ntsync_q_entry {
> + struct list_head node;
> + struct ntsync_q *q;
> + struct ntsync_obj *obj;
> + __u32 index;
> +};
> +
> +struct ntsync_q {
> + struct task_struct *task;
> + __u32 owner;
> +
> + /*
> + * Protected via atomic_try_cmpxchg(). Only the thread that wins the
> + * compare-and-swap may actually change object states and wake this
> + * task.
> + */
> + atomic_t signaled;
This feels odd, why are you duplicating a normal lock functionality
here?
> +
> + __u32 count;
> + struct ntsync_q_entry entries[];
> };
>
> struct ntsync_device {
> struct file *file;
> };
>
> +static void try_wake_any_sem(struct ntsync_obj *sem)
> +{
> + struct ntsync_q_entry *entry;
> +
> + lockdep_assert_held(&sem->lock);
> +
> + list_for_each_entry(entry, &sem->any_waiters, node) {
> + struct ntsync_q *q = entry->q;
> + int signaled = -1;
> +
> + if (!sem->u.sem.count)
> + break;
> +
> + if (atomic_try_cmpxchg(&q->signaled, &signaled, entry->index)) {
> + sem->u.sem.count--;
> + wake_up_process(q->task);
> + }
You are waking up _all_ "locks" that with the atomic_try_cmpxchg() call,
right? Not just the "first".
Or am I confused?
> + }
> +}
> +
> /*
> * Actually change the semaphore state, returning -EOVERFLOW if it is made
> * invalid.
> @@ -88,6 +139,8 @@ static int ntsync_sem_post(struct ntsync_obj *sem, void __user *argp)
>
> prev_count = sem->u.sem.count;
> ret = post_sem_state(sem, args);
> + if (!ret)
> + try_wake_any_sem(sem);
>
> spin_unlock(&sem->lock);
>
> @@ -141,6 +194,7 @@ static struct ntsync_obj *ntsync_alloc_obj(struct ntsync_device *dev,
> obj->dev = dev;
> get_file(dev->file);
> spin_lock_init(&obj->lock);
> + INIT_LIST_HEAD(&obj->any_waiters);
>
> return obj;
> }
> @@ -191,6 +245,200 @@ static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
> return put_user(fd, &user_args->sem);
> }
>
> +static struct ntsync_obj *get_obj(struct ntsync_device *dev, int fd)
> +{
> + struct file *file = fget(fd);
> + struct ntsync_obj *obj;
> +
> + if (!file)
> + return NULL;
> +
> + if (file->f_op != &ntsync_obj_fops) {
> + fput(file);
> + return NULL;
> + }
> +
> + obj = file->private_data;
> + if (obj->dev != dev) {
> + fput(file);
> + return NULL;
> + }
> +
> + return obj;
> +}
> +
> +static void put_obj(struct ntsync_obj *obj)
> +{
> + fput(obj->file);
> +}
> +
> +static int ntsync_schedule(const struct ntsync_q *q, const struct ntsync_wait_args *args)
> +{
> + ktime_t timeout = ns_to_ktime(args->timeout);
> + clockid_t clock = CLOCK_MONOTONIC;
> + ktime_t *timeout_ptr;
> + int ret = 0;
> +
> + timeout_ptr = (args->timeout == U64_MAX ? NULL : &timeout);
> +
> + if (args->flags & NTSYNC_WAIT_REALTIME)
> + clock = CLOCK_REALTIME;
> +
> + do {
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + break;
> + }
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (atomic_read(&q->signaled) != -1) {
> + ret = 0;
> + break;
What happens if the value changes right after you read it?
Rolling your own lock is tricky, and needs review from the locking
maintainers. And probably some more documentation as to what is
happening and why our normal types of locks can't be used here?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-04-11 7:11 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: Thomas Gleixner, richardcochran, luto, mingo, bp, dave.hansen,
x86, hpa, arnd, geert, peterz, hannes, sohil.mehta,
rick.p.edgecombe, nphamcs, palmer, keescook, legion, mark.rutland,
mszeredi, casey, reibax, davem, brauner, linux-kernel, linux-api,
linux-arch, netdev
In-Reply-To: <CAF2d9jikELOQa_9Kk+oF_=_7NZTn9DuAw=s9KQR6-EfWTiW5RQ@mail.gmail.com>
Hi Mahesh
What is the status of your patch?
if your patch is upstreamed , then it will have all I need.
But, If not , I will upstream my patch.
BR,
On Thu, Apr 11, 2024 at 5:56 AM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> > > On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > The modification that you have proposed (in a couple of posts back)
> > > would work but it's still not ideal since the pre/post ts are not
> > > close enough as they are currently (properly implemented!)
> > > gettimex64() would have. The only way to do that would be to have
> > > another ioctl as I have proposed which is a superset of current
> > > gettimex64 and pre-post collection is the closest possible.
> >
> > Errm. What I posted as sketch _is_ using gettimex64() with the extra
> > twist of the flag vs. a clockid (which is an implementation detail) and
> > the difference that I carry the information in ptp_system_timestamp
> > instead of needing a new argument clockid to all existing callbacks
> > because the modification to ptp_read_prets() and postts() will just be
> > sufficient, no?
> >
> OK, that makes sense.
>
> > For the case where the driver does not provide gettimex64() then the
> > extension of the original offset ioctl is still providing a better
> > mechanism than the proposed syscall.
> >
> > I also clearly said that all drivers should be converted over to
> > gettimex64().
> >
> I agree. Honestly that should have been mandatory and
> ptp_register_clock() should fail otherwise! Probably should have been
> part of gettimex64 implementation :(
>
> I don't think we can do anything other than just hoping all driver
> implementations include gettimex64 implementation.
>
> > > Having said that, the 'flag' modification proposal is a good backup
> > > for the drivers that don't have good implementation (close enough but
> > > not ideal). Also, you don't need a new ioctl-op. So if we really want
> > > precision, I believe, we need a new ioctl op (with supporting
> > > implementation similar to the mlx4 code above). but we want to save
> > > the new ioctl-op and have less precision then proposed modification
> > > would work fine.
> >
> > I disagree. The existing gettimex64() is good enough if the driver
> > implements it correctly today. If not then those drivers need to be
> > fixed independent of this.
> >
> > So assumed that a driver does:
> >
> > gettimex64()
> > ptp_prets(sts);
> > read_clock();
> > ptp_postts(sts);
> >
> > today then having:
> >
> > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > {
> > if (sts) {
> > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > ktime_get_raw_ts64(&sts->pre_ts);
> > else
> > ktime_get_real_ts64(&sts->pre_ts);
> > }
> > }
> >
> > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > {
> > if (sts) {
> > if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> > ktime_get_raw_ts64(&sts->post_ts);
> > else
> > ktime_get_real_ts64(&sts->post_ts);
> > }
> > }
> >
> > or
> >
> > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > {
> > if (sts) {
> > switch (sts->clockid) {
> > case CLOCK_MONOTONIC_RAW:
> > time_get_raw_ts64(&sts->pre_ts);
> > break;
> > case CLOCK_REALTIME:
> > ktime_get_real_ts64(&sts->pre_ts);
> > break;
> > }
> > }
> > }
> >
> > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > {
> > if (sts) {
> > switch (sts->clockid) {
> > case CLOCK_MONOTONIC_RAW:
> > time_get_raw_ts64(&sts->post_ts);
> > break;
> > case CLOCK_REALTIME:
> > ktime_get_real_ts64(&sts->post_ts);
> > break;
> > }
> > }
> > }
> >
> > is doing the exact same thing as your proposal but without touching any
> > driver which implements gettimex64() correctly at all.
> >
> I see. Yes, this makes sense.
>
> > While your proposal requires to touch every single driver for no reason,
> > no?
> >
> > It is just an implementation detail whether you use a flag or a
> > clockid. You can carry the clockid for the clocks which actually can be
> > read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
> >
> > struct ptp_sys_offset_extended {
> > unsigned int n_samples; /* Desired number of measurements. */
> > clockid_t clockid;
> > unsigned int rsv[2]; /* Reserved for future use. */
> > };
> >
> > and in the IOCTL:
> >
> > if (extoff->clockid != CLOCK_MONOTONIC_RAW)
> > return -EINVAL;
> >
> > sts.clockid = extoff->clockid;
> >
> > and it all just works, no?
> >
> Yes, this should work. However, I didn't check if struct
> ptp_system_timestamp is used in some other context.
>
> > I have no problem to decide that PTP_SYS_OFFSET will not get this
> > treatment and the drivers have to be converted over to
> > PTP_SYS_OFFSET_EXTENDED.
> >
> > But adding yet another callback just to carry a clockid as argument is a
> > more than pointless exercise as I demonstrated.
> >
> Agreed. As I said, I thought we cannot change the gettimex64() without
> breaking the compatibility but the fact that CLOCK_REALTIME is "0"
> works well for the backward compatibility case.
>
> I can spin up an updated patch/series that updates gettimex64
> implementation instead of adding a new ioctl-op If you all agree.
>
> thanks,
> --mahesh..
>
> > Thanks,
> >
> > tglx
^ permalink raw reply
* Re: [PATCH v4 2/3] VT: Add KDFONTINFO ioctl
From: Jiri Slaby @ 2024-04-11 3:53 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial, Helge Deller
In-Reply-To: <Zha_-zPHCW8iYT_4@example.org>
On 10. 04. 24, 18:36, Alexey Gladkov wrote:
> On Wed, Apr 03, 2024 at 07:05:14AM +0200, Jiri Slaby wrote:
>> First, there was no need to send this v4 so quickly. Provided we have
>> not settled in v3... This makes the review process painful.
>>
>> And then:
>>
>> On 02. 04. 24, 19:50, Alexey Gladkov wrote:
>>> Each driver has its own restrictions on font size. There is currently no
>>> way to understand what the requirements are. The new ioctl allows
>>> userspace to get the minimum and maximum font size values.
>>>
>>> Acked-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Alexey Gladkov <legion@kernel.org>
>> ...
>>> --- a/drivers/tty/vt/vt_ioctl.c
>>> +++ b/drivers/tty/vt/vt_ioctl.c
>>> @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>>> break;
>>> }
>>>
>>> + case KDFONTINFO: {
>>> + struct console_font_info fnt_info;
>>> +
>>> + ret = con_font_info(vc, &fnt_info);
>>> + if (ret)
>>> + return ret;
>>> + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>>
>> sizeof, I already commented.
>
> I'm not sure I understand. sizeof(*up), but 'up' is 'void __user *up'.
I don't know what I saw/was thinking previously, ignore this one :).
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Mahesh Bandewar (महेश बंडेवार) @ 2024-04-11 2:55 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Sagi Maimon, richardcochran, luto, mingo, bp, dave.hansen, x86,
hpa, arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <871q7md0ak.ffs@tglx>
On Wed, Apr 3, 2024 at 6:48 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Apr 02 2024 at 16:37, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > The modification that you have proposed (in a couple of posts back)
> > would work but it's still not ideal since the pre/post ts are not
> > close enough as they are currently (properly implemented!)
> > gettimex64() would have. The only way to do that would be to have
> > another ioctl as I have proposed which is a superset of current
> > gettimex64 and pre-post collection is the closest possible.
>
> Errm. What I posted as sketch _is_ using gettimex64() with the extra
> twist of the flag vs. a clockid (which is an implementation detail) and
> the difference that I carry the information in ptp_system_timestamp
> instead of needing a new argument clockid to all existing callbacks
> because the modification to ptp_read_prets() and postts() will just be
> sufficient, no?
>
OK, that makes sense.
> For the case where the driver does not provide gettimex64() then the
> extension of the original offset ioctl is still providing a better
> mechanism than the proposed syscall.
>
> I also clearly said that all drivers should be converted over to
> gettimex64().
>
I agree. Honestly that should have been mandatory and
ptp_register_clock() should fail otherwise! Probably should have been
part of gettimex64 implementation :(
I don't think we can do anything other than just hoping all driver
implementations include gettimex64 implementation.
> > Having said that, the 'flag' modification proposal is a good backup
> > for the drivers that don't have good implementation (close enough but
> > not ideal). Also, you don't need a new ioctl-op. So if we really want
> > precision, I believe, we need a new ioctl op (with supporting
> > implementation similar to the mlx4 code above). but we want to save
> > the new ioctl-op and have less precision then proposed modification
> > would work fine.
>
> I disagree. The existing gettimex64() is good enough if the driver
> implements it correctly today. If not then those drivers need to be
> fixed independent of this.
>
> So assumed that a driver does:
>
> gettimex64()
> ptp_prets(sts);
> read_clock();
> ptp_postts(sts);
>
> today then having:
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
> if (sts) {
> if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> ktime_get_raw_ts64(&sts->pre_ts);
> else
> ktime_get_real_ts64(&sts->pre_ts);
> }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
> if (sts) {
> if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> ktime_get_raw_ts64(&sts->post_ts);
> else
> ktime_get_real_ts64(&sts->post_ts);
> }
> }
>
> or
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
> if (sts) {
> switch (sts->clockid) {
> case CLOCK_MONOTONIC_RAW:
> time_get_raw_ts64(&sts->pre_ts);
> break;
> case CLOCK_REALTIME:
> ktime_get_real_ts64(&sts->pre_ts);
> break;
> }
> }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
> if (sts) {
> switch (sts->clockid) {
> case CLOCK_MONOTONIC_RAW:
> time_get_raw_ts64(&sts->post_ts);
> break;
> case CLOCK_REALTIME:
> ktime_get_real_ts64(&sts->post_ts);
> break;
> }
> }
> }
>
> is doing the exact same thing as your proposal but without touching any
> driver which implements gettimex64() correctly at all.
>
I see. Yes, this makes sense.
> While your proposal requires to touch every single driver for no reason,
> no?
>
> It is just an implementation detail whether you use a flag or a
> clockid. You can carry the clockid for the clocks which actually can be
> read in that context in a reserved field of PTP_SYS_OFFSET_EXTENDED:
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> clockid_t clockid;
> unsigned int rsv[2]; /* Reserved for future use. */
> };
>
> and in the IOCTL:
>
> if (extoff->clockid != CLOCK_MONOTONIC_RAW)
> return -EINVAL;
>
> sts.clockid = extoff->clockid;
>
> and it all just works, no?
>
Yes, this should work. However, I didn't check if struct
ptp_system_timestamp is used in some other context.
> I have no problem to decide that PTP_SYS_OFFSET will not get this
> treatment and the drivers have to be converted over to
> PTP_SYS_OFFSET_EXTENDED.
>
> But adding yet another callback just to carry a clockid as argument is a
> more than pointless exercise as I demonstrated.
>
Agreed. As I said, I thought we cannot change the gettimex64() without
breaking the compatibility but the fact that CLOCK_REALTIME is "0"
works well for the backward compatibility case.
I can spin up an updated patch/series that updates gettimex64
implementation instead of adding a new ioctl-op If you all agree.
thanks,
--mahesh..
> Thanks,
>
> tglx
^ permalink raw reply
* [PATCH] uapi: add missing const qualifier to cast in some byteorder functions
From: arlr @ 2024-04-10 20:54 UTC (permalink / raw)
To: linux-kernel, linux-api
Add missing const qualifiers to cast in __{le,be}{16,32,64}_to_cpup.
Signed-off-by: Carl Rosdahl <carl.rosdahl05@gmail.com>
---
reduced testcase (on exported headers):
$ cc -c -o /dev/null -Wcast-qual -xc usr/include/linux/byteorder/big_endian.h
$ cc -c -o /dev/null -Wcast-qual -xc usr/include/linux/byteorder/little_endian.h
diff --git a/include/uapi/linux/byteorder/big_endian.h
b/include/uapi/linux/byteorder/big_endian.h
index 80aa5c41a763..e59ec4f8bd92 100644
--- a/include/uapi/linux/byteorder/big_endian.h
+++ b/include/uapi/linux/byteorder/big_endian.h
@@ -48,7 +48,7 @@ static __always_inline __le64 __cpu_to_le64p(const __u64 *p)
}
static __always_inline __u64 __le64_to_cpup(const __le64 *p)
{
- return __swab64p((__u64 *)p);
+ return __swab64p((const __u64 *)p);
}
static __always_inline __le32 __cpu_to_le32p(const __u32 *p)
{
@@ -56,7 +56,7 @@ static __always_inline __le32 __cpu_to_le32p(const __u32 *p)
}
static __always_inline __u32 __le32_to_cpup(const __le32 *p)
{
- return __swab32p((__u32 *)p);
+ return __swab32p((const __u32 *)p);
}
static __always_inline __le16 __cpu_to_le16p(const __u16 *p)
{
@@ -64,7 +64,7 @@ static __always_inline __le16 __cpu_to_le16p(const __u16 *p)
}
static __always_inline __u16 __le16_to_cpup(const __le16 *p)
{
- return __swab16p((__u16 *)p);
+ return __swab16p((const __u16 *)p);
}
static __always_inline __be64 __cpu_to_be64p(const __u64 *p)
{
diff --git a/include/uapi/linux/byteorder/little_endian.h
b/include/uapi/linux/byteorder/little_endian.h
index cd98982e7523..65204e600fbf 100644
--- a/include/uapi/linux/byteorder/little_endian.h
+++ b/include/uapi/linux/byteorder/little_endian.h
@@ -72,7 +72,7 @@ static __always_inline __be64 __cpu_to_be64p(const __u64 *p)
}
static __always_inline __u64 __be64_to_cpup(const __be64 *p)
{
- return __swab64p((__u64 *)p);
+ return __swab64p((const __u64 *)p);
}
static __always_inline __be32 __cpu_to_be32p(const __u32 *p)
{
@@ -80,7 +80,7 @@ static __always_inline __be32 __cpu_to_be32p(const __u32 *p)
}
static __always_inline __u32 __be32_to_cpup(const __be32 *p)
{
- return __swab32p((__u32 *)p);
+ return __swab32p((const __u32 *)p);
}
static __always_inline __be16 __cpu_to_be16p(const __u16 *p)
{
@@ -88,7 +88,7 @@ static __always_inline __be16 __cpu_to_be16p(const __u16 *p)
}
static __always_inline __u16 __be16_to_cpup(const __be16 *p)
{
- return __swab16p((__u16 *)p);
+ return __swab16p((const __u16 *)p);
}
#define __cpu_to_le64s(x) do { (void)(x); } while (0)
#define __le64_to_cpus(x) do { (void)(x); } while (0)
^ permalink raw reply related
* Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Greg Kroah-Hartman @ 2024-04-10 17:11 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Jiri Slaby, LKML, kbd, linux-api, linux-fbdev, linux-serial,
Helge Deller
In-Reply-To: <Zha-X8QS_8L9eF0r@example.org>
On Wed, Apr 10, 2024 at 06:29:19PM +0200, Alexey Gladkov wrote:
> On Wed, Apr 03, 2024 at 07:27:55AM +0200, Jiri Slaby wrote:
> > On 02. 04. 24, 15:19, Alexey Gladkov wrote:
> > >>> --- a/include/uapi/linux/kd.h
> > >>> +++ b/include/uapi/linux/kd.h
> > ...
> > >>> +struct console_font_info {
> > >>> + unsigned int min_width, min_height; /* minimal font size */
> > >>> + unsigned int max_width, max_height; /* maximum font size */
> > >>> + unsigned int flags; /* KD_FONT_INFO_FLAG_* */
> > >>
> > >> This does not look like a well-defined™ and extendable uapi structure.
> > >> While it won't change anything here, still use fixed-length __u32.
> > >>
> > >> And you should perhaps add some reserved fields. Do not repeat the same
> > >> mistakes as your predecessors with the current kd uapi.
> > >
> > > I thought about it, but I thought it would be overengineering.
> >
> > It would not. UAPI structs are set in stone once released.
> >
> > And in this case, it's likely you would want to know more info about
> > fonts in the future.
> >
> > > Can you suggest how best to do this?
> >
> > Given you have flags in there already (to state that the structure
> > contains more), just add an array of u32 reserved[] space. 3 or 5, I
> > would say (to align the struct to 64bit).
>
> struct console_font_info {
> __u32 min_width, min_height; /* minimal font size */
> __u32 max_width, max_height; /* maximum font size */
> __u32 flags; /* KD_FONT_INFO_FLAG_* */
> __u32 reserved[5]; /* This field is reserved forfuture use. Must be 0. */
> };
>
> So, struct should be like this ?
>
> I wouldn't add the version to the flags. Maybe it would be better to add a
> separate field with the version?
Versions do not work for ioctls, just use flags for stuff. And you
might want to put flags first?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4 2/3] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-10 16:36 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial, Helge Deller
In-Reply-To: <211f3c45-7064-475b-b9e1-f6adbbba8879@kernel.org>
On Wed, Apr 03, 2024 at 07:05:14AM +0200, Jiri Slaby wrote:
> First, there was no need to send this v4 so quickly. Provided we have
> not settled in v3... This makes the review process painful.
>
> And then:
>
> On 02. 04. 24, 19:50, Alexey Gladkov wrote:
> > Each driver has its own restrictions on font size. There is currently no
> > way to understand what the requirements are. The new ioctl allows
> > userspace to get the minimum and maximum font size values.
> >
> > Acked-by: Helge Deller <deller@gmx.de>
> > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> ...
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -479,6 +479,17 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> > break;
> > }
> >
> > + case KDFONTINFO: {
> > + struct console_font_info fnt_info;
> > +
> > + ret = con_font_info(vc, &fnt_info);
> > + if (ret)
> > + return ret;
> > + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>
> sizeof, I already commented.
I'm not sure I understand. sizeof(*up), but 'up' is 'void __user *up'.
> Now you leak info to userspace unless everyone sets everything in
> fnt_info. IOW, do memset() above.
Yes. I miss it. Sorry.
--
Rgrds, legion
^ permalink raw reply
* Re: [RESEND PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-10 16:29 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, LKML, kbd, linux-api, linux-fbdev,
linux-serial, Helge Deller
In-Reply-To: <6bb4f4fb-573c-4f63-967c-2cb08514fc91@kernel.org>
On Wed, Apr 03, 2024 at 07:27:55AM +0200, Jiri Slaby wrote:
> On 02. 04. 24, 15:19, Alexey Gladkov wrote:
> >>> --- a/include/uapi/linux/kd.h
> >>> +++ b/include/uapi/linux/kd.h
> ...
> >>> +struct console_font_info {
> >>> + unsigned int min_width, min_height; /* minimal font size */
> >>> + unsigned int max_width, max_height; /* maximum font size */
> >>> + unsigned int flags; /* KD_FONT_INFO_FLAG_* */
> >>
> >> This does not look like a well-defined™ and extendable uapi structure.
> >> While it won't change anything here, still use fixed-length __u32.
> >>
> >> And you should perhaps add some reserved fields. Do not repeat the same
> >> mistakes as your predecessors with the current kd uapi.
> >
> > I thought about it, but I thought it would be overengineering.
>
> It would not. UAPI structs are set in stone once released.
>
> And in this case, it's likely you would want to know more info about
> fonts in the future.
>
> > Can you suggest how best to do this?
>
> Given you have flags in there already (to state that the structure
> contains more), just add an array of u32 reserved[] space. 3 or 5, I
> would say (to align the struct to 64bit).
struct console_font_info {
__u32 min_width, min_height; /* minimal font size */
__u32 max_width, max_height; /* maximum font size */
__u32 flags; /* KD_FONT_INFO_FLAG_* */
__u32 reserved[5]; /* This field is reserved forfuture use. Must be 0. */
};
So, struct should be like this ?
I wouldn't add the version to the flags. Maybe it would be better to add a
separate field with the version?
--
Rgrds, legion
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-10 11:45 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <66109191f3d6d_2583ad29468@dwillia2-xfh.jf.intel.com.notmuch>
On Fri, 5 Apr 2024 17:04:34 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
Hi Dan,
> Jonathan Cameron wrote:
> > Hi All,
> >
> > This is has come up in a number of discussions both on list and in private,
> > so I wanted to lay out a potential set of rules when deciding whether or not
> > to provide a user space interface for a particular feature of CXL Fabric
> > Management. The intent is to drive discussion, not to simply tell people
> > a set of rules. I've brought this to the public lists as it's a Linux kernel
> > policy discussion, not a standards one.
> >
> > Whilst I'm writing the RFC this my attempt to summarize a possible
> > position rather than necessarily being my personal view.
> >
> > It's a straw man - shoot at it!
> >
> > Not everyone in this discussion is familiar with relevant kernel or CXL concepts
> > so I've provided more info than I normally would.
>
> Thanks for writing this up Jonathan!
>
> [..]
> > 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> > ==========================================================================
> >
> > (This would just be a kernel option that we'd advise normal server
> > distributions not to turn on. Would be enabled by openBMC etc)
> >
> > This is fine - there is some work to do, but the switch-cci PCI driver
> > will hopefully be ready for upstream merge soon. There is no filtering of
> > accesses. Think of this as similar to all the damage you can do via
> > MCTP from a BMC. Similarly it is likely that much of the complexity
> > of the actual commands will be left to user space tooling:
> > https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
> >
> > Whether Kconfig help text is strong enough to ensure this only gets
> > enabled for BMC targeted distros is an open question we can address
> > alongside an updated patch set.
>
> It is not clear to me that this material makes sense to house in
> drivers/ vs tools/ or even out-of-tree just for maintenance burden
> relief of keeping the universes separated. What does the Linux kernel
> project get out of carrying this in mainline alongside the inband code?
I'm not sure what you mean by in band. Aim here was to discuss
in-band drivers for switch CCI etc. Same reason from a kernel point of
view for why we include embedded drivers. I'll interpret in band
as host driven and not inband as FM-API stuff.
> I do think the mailbox refactoring to support non-CXL use cases is
> interesting, but only so far as refactoring is consumed for inband use
> cases like RAS API.
If I read this right, I disagree with the 'only so far' bit.
In all substantial ways we should support BMC use case of the Linux Kernel
at a similar level to how we support forms of Linux Distros. It may
not be our target market as developers for particular parts of our companies,
but we should not block those who want to support it.
We should support them in drivers/ - maybe with example userspace code
in tools. Linux distros on BMCs is a big market, there are a number
of different distros using (and in some cases contributing to) the
upstream kernel. Not everyone is using openBMC so there is not one
common place where downstream patches could be carried.
From a personal point of view, I like that for the same reasons that
I like there being multiple Linux sever focused distros. It's a sign
of a healthy ecosystem to have diverse options taking the mainline
kernel as their starting point.
BMCs are just another embedded market, and like other embedded markets
we want to encourage upstream first etc.
openBMC has a policy on this:
https://github.com/openbmc/docs/blob/master/kernel-development.md
"The OpenBMC project maintains a kernel tree for use by the project.
The tree's general development policy is that code must be upstream
first." There are paths to bypass that for openBMC so it's a little
more relaxed than some enterprise distros (today, their policies used
to look very similar to this) but we should not be telling
them they need to carry support downstream. If we are
going to tell them that, we need to be able to point at a major
sticking point for maintenance burden. So far I don't see the
additional complexity as remotely close reaching that bar.
So I think we do want switch-cci support and for that matter the equivalent
for MHDs in the upstream kernel.
One place I think there is some wiggle room is the taint on use of raw
commands. Leaving removal of that for BMC kernels as a patch they need
to carry downstream doesn't seem too burdensome. I'm sure they'll push
back if it is a problem for them! So I think we can kick that question
into the future.
Addressing maintenance burden, there is a question of where we split
the stack. Ignore MHDs for now (I won't go into why in this forum...)
The current proposal is (simplified to ignore some sharing in lookup code etc
that I can rip out if we think it might be a long term problem)
_____________ _____________________
| | | |
| Switch CCI | | Type 3 Driver stack|
|_____________| |_____________________|
|___________________________| Whatever GPU etc
_______|_______ _______|______
| | | |
| CXL MBOX | | RAS API etc |
|_______________| |______________|
|_____________________________|
|
_________|______
| |
| MMPT mbox |
|________________|
Switch CCI Driver: PCI driver doing everything beyond the CXL mbox specific bit.
Type 3 Stack: All the normal stack just with the CXL Mailbox specific stuff factored
out. Note we can move different amounts of shared logic in here, but
in essence it deals with the extra layer on top of the raw MMPT mbox.
MMPT Mbox: Mailbox as per the PCI spec.
RAS API: Shared RAS API specific infrastructure used by other drivers.
If we see a significant maintenance burden, maybe we duplicate the CXL specific
MBOX layer - I can see advantages in that as there is some stuff not relevant
to the Switch CCI. There will be some duplication of logic however such
as background command support (which is CXL only IIUC) We can even use
a difference IOCTL number so the two can diverge if needed in the long run.
e.g. If it makes it easier to get upstream, we can merrily duplicated code
so that only the bit common with RAS API etc is shared (assuming the
actually end up with MMPT, not the CXL mailbox which is what their current
publicly available spec talks about and I assume is a pref MMPT left over?)
_____________ _____________________
| | | |
| Switch CCI | | Type 3 Driver stack|
|_____________| |_____________________|
| | Whatever GPU etc
_______|_______ _______|_______ ______|_______
| | | | | |
| CXL MBOX | | CXL MBOX | | RAS API etc |
|_______________| |_______________| |______________|
|_____________________________|____________________|
|
________|______
| |
| MMPT mbox |
|_______________|
> > (On to the one that the "debate" is about)
> >
> > 3) Unfiltered user space use of mailbox for Fabric Management - Distro kernels
> > =============================================================================
> > (General purpose Linux Server Distro (Redhat, Suse etc))
> >
> > This is equivalent of RAW command support on CXL Type 3 memory devices.
> > You can enable those in a distro kernel build despite the scary config
> > help text, but if you use it the kernel is tainted. The result
> > of the taint is to add a flag to bug reports and print a big message to say
> > that you've used a feature that might result in you shooting yourself
> > in the foot.
> >
> > The taint is there because software is not at first written to deal with
> > everything that can happen smoothly (e.g. surprise removal) It's hard
> > to survive some of these events, so is never on the initial feature list
> > for any bus, so this flag is just to indicate we have entered a world
> > where almost all bets are off wrt to stability. We might not know what
> > a command does so we can't assess the impact (and no one trusts vendor
> > commands to report affects right in the Command Effects Log - which
> > in theory tells you if a command can result problems).
>
> That is a secondary reason that the taint is there. Yes, it helps
> upstream not waste their time on bug reports from proprietary use cases,
> but the effect of that is to make "raw" command mode unattractive for
> deploying solutions at scale. It clarifies that this interface is a
> debug-tool that enterprise environment need not worry about.
>
> The more salient reason for the taint, speaking only for myself as a
> Linux kernel community member not for $employer, is to encourage open
> collaboration. Take firmware-update for example that is a standard
> command with known side effects that is inaccessible via the ioctl()
> path. It is placed behind an ABI that is easier to maintain and reason
> about. Everyone has the firmware update tool if they have the 'cat'
> command. Distros appreciate the fact that they do not need ship yet
> another vendor device-update tool, vendors get free tooling and end
> users also appreciate one flow for all devices.
>
> As I alluded here [1], I am not against innovation outside of the
> specification, but it needs to be open, and it needs to plausibly become
> if not a de jure standard at least a de facto standard.
>
> [1]: https://lore.kernel.org/all/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
Agree with all this.
>
> > A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> > (a r3.1 feature) but, as I understand it, these are intended for a
> > host to configure and should not have side effects on other hosts?
> > My working assumption is that the kernel driver stack will handle
> > these (once we catch up with the current feature backlog!) Currently
> > we have no visibility of what the OS driver stack for a fabrics will
> > actually look like - the spec is just the starting point for that.
> > (patches welcome ;)
> >
> > The various CXL upstream developers and maintainers may have
> > differing views of course, but my current understanding is we want
> > to support 1 and 2, but are very resistant to 3!
>
> 1, yes, 2, need to see the patches, and agree on 3.
If we end up with top architecture of the diagrams above, 2 will look pretty
similar to last version of the switch-cci patches. So raw commands only + taint.
Factoring out MMPT is another layer that doesn't make that much difference in
practice to this discussion. Good to have, but the reuse here would be one layer
above that.
Or we just say go for second proposed architecture and 0 impact on the
CXL specific code, just reuse of the MMPT layer. I'd imagine people will get
grumpy on code duplication (and we'll spend years rejecting patch sets that
try to share the cdoe) but there should be no maintenance burden as
a result.
>
> > General Notes
> > =============
> >
> > One side aspect of why we really don't like unfiltered userspace access to any
> > of these devices is that people start building non standard hacks in and we
> > lose the ecosystem advantages. Forcing a considered discussion + patches
> > to let a particular command be supported, drives standardization.
>
> Like I said above, I think this is not a side aspect. It is fundamental
> to the viability Linux as a project. This project only works because
> organizations with competing goals realize they need some common
> infrastructure and that there is little to be gained by competing on the
> commons.
>
> > https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> > provides some history on vendor specific extensions and why in general we
> > won't support them upstream.
>
> Oh, you linked my writeup... I will leave the commentary I added here in case
> restating it helps.
>
> > To address another question raised in an earlier discussion:
> > Putting these Fabric Management interfaces behind guard rails of some type
> > (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> > of non standard interfaces, because we will be even less likely to accept
> > those upstream!
> >
> > If anyone needs more details on any aspect of this please ask.
> > There are a lot of things involved and I've only tried to give a fairly
> > minimal illustration to drive the discussion. I may well have missed
> > something crucial.
>
> You captured it well, and this is open source so I may have missed
> something crucial as well.
>
Thanks for detailed reply!
Jonathan
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-04-09 12:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240408162258.GC25058@redhat.com>
On Mon, Apr 08, 2024 at 06:22:59PM +0200, Oleg Nesterov wrote:
> On 04/08, Jiri Olsa wrote:
> >
> > On Fri, Apr 05, 2024 at 01:02:30PM +0200, Oleg Nesterov wrote:
> > >
> > > And what should sys_uretprobe() do if it is not called from the trampoline?
> > > I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
> >
> > so the similar behaviour with int3 ends up with immediate SIGTRAP
> > and not invoking pending uretprobe consumers, like:
> >
> > - setup uretprobe for foo
> > - foo() {
> > executes int 3 -> sends SIGTRAP
> > }
> >
> > because the int3 handler checks if it got executed from the uretprobe's
> > trampoline.
>
> ... or the task has uprobe at this address
>
> > if not it treats that int3 as regular trap
>
> Yes this mimics the "default" behaviour without uprobes/uretprobes
>
> > so I think we should mimic int3 behaviour and:
> >
> > - setup uretprobe for foo
> > - foo() {
> > uretprobe_syscall -> check if we got executed from uretprobe's
> > trampoline and send SIGILL if that's not the case
>
> Agreed,
>
> > I think it's better to have the offending process killed right away,
> > rather than having more undefined behaviour, waiting for final 'ret'
> > instruction that jumps to uretprobe trampoline and causes SIGILL
>
> Agreed. In fact I think it should be also killed if copy_to/from_user()
> fails by the same reason.
+1 makes sense
jirka
>
> Oleg.
>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-04-09 7:57 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Jiri Olsa, Oleg Nesterov, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240409093439.3906e3783ab1f5280146934e@kernel.org>
On Tue, Apr 09, 2024 at 09:34:39AM +0900, Masami Hiramatsu wrote:
SNIP
> > >
> > > > this can be fixed by checking the syscall is called from the trampoline
> > > > and prevent handle_trampoline call if it's not
> > >
> > > Yes, but I still do not think this makes a lot of sense. But I won't argue.
> > >
> > > And what should sys_uretprobe() do if it is not called from the trampoline?
> > > I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
> >
> > so the similar behaviour with int3 ends up with immediate SIGTRAP
> > and not invoking pending uretprobe consumers, like:
> >
> > - setup uretprobe for foo
> > - foo() {
> > executes int 3 -> sends SIGTRAP
> > }
> >
> > because the int3 handler checks if it got executed from the uretprobe's
> > trampoline.. if not it treats that int3 as regular trap
>
> Yeah, that is consistent behavior. Sounds good to me.
>
> >
> > while for uretprobe syscall we have at the moment following behaviour:
> >
> > - setup uretprobe for foo
> > - foo() {
> > uretprobe_syscall -> executes foo's uretprobe consumers
> > }
> > - at some point we get to the 'ret' instruction that jump into uretprobe
> > trampoline and the uretprobe_syscall won't find pending uretprobe and
> > will send SIGILL
> >
> >
> > so I think we should mimic int3 behaviour and:
> >
> > - setup uretprobe for foo
> > - foo() {
> > uretprobe_syscall -> check if we got executed from uretprobe's
> > trampoline and send SIGILL if that's not the case
>
> OK, this looks good to me.
>
> >
> > I think it's better to have the offending process killed right away,
> > rather than having more undefined behaviour, waiting for final 'ret'
> > instruction that jumps to uretprobe trampoline and causes SIGILL
> >
> > >
> > > I agree very much with Andrii,
> > >
> > > sigreturn() exists only to allow the implementation of signal handlers. It should never be
> > > called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
> > > the architecture.
> > >
> > > this is how sys_uretprobe() should be treated/documented.
> >
> > yes, will include man page patch in new version
>
> And please follow Documentation/process/adding-syscalls.rst in new version,
> then we can avoid repeating the same discussion :-)
yep, will do
thanks,
jirka
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-09 0:34 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <ZhQVBYQYr5ph33Uu@krava>
On Mon, 8 Apr 2024 18:02:13 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Fri, Apr 05, 2024 at 01:02:30PM +0200, Oleg Nesterov wrote:
> > On 04/05, Jiri Olsa wrote:
> > >
> > > On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> > > >
> > > > I think this expects setjmp/longjmp as below
> > > >
> > > > foo() { <- retprobe1
> > > > setjmp()
> > > > bar() { <- retprobe2
> > > > longjmp()
> > > > }
> > > > } <- return to trampoline
> > > >
> > > > In this case, we need to skip retprobe2's instance.
> >
> > Yes,
> >
> > > > My concern is, if we can not find appropriate return instance, what happen?
> > > > e.g.
> > > >
> > > > foo() { <-- retprobe1
> > > > bar() { # sp is decremented
> > > > sys_uretprobe() <-- ??
> > > > }
> > > > }
> > > >
> > > > It seems sys_uretprobe() will handle retprobe1 at that point instead of
> > > > SIGILL.
> > >
> > > yes, and I think it's fine, you get the consumer called in wrong place,
> > > but it's your fault and kernel won't crash
> >
> > Agreed.
> >
> > With or without this patch userpace can also do
> >
> > foo() { <-- retprobe1
> > bar() {
> > jump to xol_area
> > }
> > }
> >
> > handle_trampoline() will handle retprobe1.
> >
> > > this can be fixed by checking the syscall is called from the trampoline
> > > and prevent handle_trampoline call if it's not
> >
> > Yes, but I still do not think this makes a lot of sense. But I won't argue.
> >
> > And what should sys_uretprobe() do if it is not called from the trampoline?
> > I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
>
> so the similar behaviour with int3 ends up with immediate SIGTRAP
> and not invoking pending uretprobe consumers, like:
>
> - setup uretprobe for foo
> - foo() {
> executes int 3 -> sends SIGTRAP
> }
>
> because the int3 handler checks if it got executed from the uretprobe's
> trampoline.. if not it treats that int3 as regular trap
Yeah, that is consistent behavior. Sounds good to me.
>
> while for uretprobe syscall we have at the moment following behaviour:
>
> - setup uretprobe for foo
> - foo() {
> uretprobe_syscall -> executes foo's uretprobe consumers
> }
> - at some point we get to the 'ret' instruction that jump into uretprobe
> trampoline and the uretprobe_syscall won't find pending uretprobe and
> will send SIGILL
>
>
> so I think we should mimic int3 behaviour and:
>
> - setup uretprobe for foo
> - foo() {
> uretprobe_syscall -> check if we got executed from uretprobe's
> trampoline and send SIGILL if that's not the case
OK, this looks good to me.
>
> I think it's better to have the offending process killed right away,
> rather than having more undefined behaviour, waiting for final 'ret'
> instruction that jumps to uretprobe trampoline and causes SIGILL
>
> >
> > I agree very much with Andrii,
> >
> > sigreturn() exists only to allow the implementation of signal handlers. It should never be
> > called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
> > the architecture.
> >
> > this is how sys_uretprobe() should be treated/documented.
>
> yes, will include man page patch in new version
And please follow Documentation/process/adding-syscalls.rst in new version,
then we can avoid repeating the same discussion :-)
Thank you!
>
> jirka
>
> >
> > sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> > and return -EINVAL if this addr is not valid. But why?
> >
> > Oleg.
> >
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Oleg Nesterov @ 2024-04-08 16:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <ZhQVBYQYr5ph33Uu@krava>
On 04/08, Jiri Olsa wrote:
>
> On Fri, Apr 05, 2024 at 01:02:30PM +0200, Oleg Nesterov wrote:
> >
> > And what should sys_uretprobe() do if it is not called from the trampoline?
> > I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
>
> so the similar behaviour with int3 ends up with immediate SIGTRAP
> and not invoking pending uretprobe consumers, like:
>
> - setup uretprobe for foo
> - foo() {
> executes int 3 -> sends SIGTRAP
> }
>
> because the int3 handler checks if it got executed from the uretprobe's
> trampoline.
... or the task has uprobe at this address
> if not it treats that int3 as regular trap
Yes this mimics the "default" behaviour without uprobes/uretprobes
> so I think we should mimic int3 behaviour and:
>
> - setup uretprobe for foo
> - foo() {
> uretprobe_syscall -> check if we got executed from uretprobe's
> trampoline and send SIGILL if that's not the case
Agreed,
> I think it's better to have the offending process killed right away,
> rather than having more undefined behaviour, waiting for final 'ret'
> instruction that jumps to uretprobe trampoline and causes SIGILL
Agreed. In fact I think it should be also killed if copy_to/from_user()
fails by the same reason.
Oleg.
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-04-08 16:02 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240405110230.GA22839@redhat.com>
On Fri, Apr 05, 2024 at 01:02:30PM +0200, Oleg Nesterov wrote:
> On 04/05, Jiri Olsa wrote:
> >
> > On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> > >
> > > I think this expects setjmp/longjmp as below
> > >
> > > foo() { <- retprobe1
> > > setjmp()
> > > bar() { <- retprobe2
> > > longjmp()
> > > }
> > > } <- return to trampoline
> > >
> > > In this case, we need to skip retprobe2's instance.
>
> Yes,
>
> > > My concern is, if we can not find appropriate return instance, what happen?
> > > e.g.
> > >
> > > foo() { <-- retprobe1
> > > bar() { # sp is decremented
> > > sys_uretprobe() <-- ??
> > > }
> > > }
> > >
> > > It seems sys_uretprobe() will handle retprobe1 at that point instead of
> > > SIGILL.
> >
> > yes, and I think it's fine, you get the consumer called in wrong place,
> > but it's your fault and kernel won't crash
>
> Agreed.
>
> With or without this patch userpace can also do
>
> foo() { <-- retprobe1
> bar() {
> jump to xol_area
> }
> }
>
> handle_trampoline() will handle retprobe1.
>
> > this can be fixed by checking the syscall is called from the trampoline
> > and prevent handle_trampoline call if it's not
>
> Yes, but I still do not think this makes a lot of sense. But I won't argue.
>
> And what should sys_uretprobe() do if it is not called from the trampoline?
> I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
so the similar behaviour with int3 ends up with immediate SIGTRAP
and not invoking pending uretprobe consumers, like:
- setup uretprobe for foo
- foo() {
executes int 3 -> sends SIGTRAP
}
because the int3 handler checks if it got executed from the uretprobe's
trampoline.. if not it treats that int3 as regular trap
while for uretprobe syscall we have at the moment following behaviour:
- setup uretprobe for foo
- foo() {
uretprobe_syscall -> executes foo's uretprobe consumers
}
- at some point we get to the 'ret' instruction that jump into uretprobe
trampoline and the uretprobe_syscall won't find pending uretprobe and
will send SIGILL
so I think we should mimic int3 behaviour and:
- setup uretprobe for foo
- foo() {
uretprobe_syscall -> check if we got executed from uretprobe's
trampoline and send SIGILL if that's not the case
I think it's better to have the offending process killed right away,
rather than having more undefined behaviour, waiting for final 'ret'
instruction that jumps to uretprobe trampoline and causes SIGILL
>
> I agree very much with Andrii,
>
> sigreturn() exists only to allow the implementation of signal handlers. It should never be
> called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
> the architecture.
>
> this is how sys_uretprobe() should be treated/documented.
yes, will include man page patch in new version
jirka
>
> sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> and return -EINVAL if this addr is not valid. But why?
>
> Oleg.
>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-08 3:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Andrii Nakryiko, Steven Rostedt, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD), x86,
linux-api
In-Reply-To: <20240406175558.GC3060@redhat.com>
On Sat, 6 Apr 2024 19:55:59 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/06, Masami Hiramatsu wrote:
> >
> > On Fri, 5 Apr 2024 13:02:30 +0200
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > With or without this patch userpace can also do
> > >
> > > foo() { <-- retprobe1
> > > bar() {
> > > jump to xol_area
> > > }
> > > }
> > >
> > > handle_trampoline() will handle retprobe1.
> >
> > This is OK because the execution path has been changed to trampoline,
>
> Agreed, in this case the misuse is more clear. But please see below.
>
> > but the above will continue running bar() after sys_uretprobe().
>
> .. and most probably crash
Yes, unless it returns with longjmp(). (but this is rare case and
maybe malicious program.)
>
> > > sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> > > and return -EINVAL if this addr is not valid. But why?
> >
> > Because sigreturn() never returns, but sys_uretprobe() will return.
>
> You mean, sys_uretprobe() returns to the next insn after syscall.
>
> Almost certainly yes, but this is not necessarily true. If one of consumers
> changes regs->sp sys_uretprobe() "returns" to another location, just like
> sys_rt_sigreturn().
>
> That said.
>
> Masami, it is not that I am trying to prove that you are "wrong" ;) No.
>
> I see your points even if I am biased, I understand that my objections are
> not 100% "fair".
>
> I am just trying to explain why, rightly or not, I care much less about the
> abuse of sys_uretprobe().
I would like to clear that the abuse of this syscall will not possible to harm
the normal programs, and even if it is used by malicious code (e.g. injected by
stack overflow) it doesn't cause a problem. At least thsese points are cleared,
and documented. it is easier to push it as new Linux API.
Thank you,
>
> Thanks!
>
> Oleg.
>
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-08 3:16 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <Zg-8r63tPSkuhN7p@krava>
On Fri, 5 Apr 2024 10:56:15 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > Can we avoid this with below strict check?
> >
> > if (ri->stack != regs->sp + expected_offset)
> > goto sigill;
>
> hm the current uprobe 'alive' check makes sure the return_instance is above
> or at the same stack address, not sure we can match it exactly, need to think
> about that more
>
> >
> > expected_offset should be 16 (push * 3 - ret) on x64 if we ri->stack is the
> > regs->sp right after call.
>
> the syscall trampoline already updates the regs->sp before calling
> handle_trampoline
>
> regs->sp += sizeof(r11_cx_ax);
Yes, that is "push * 3" part. And "- ret" is that the stack entry is consumed
by the "ret", which is stored by call.
1: |--------| <- sp at function entry == ri->stack
0: |ret-addr| <- call pushed it
0: |ret-addr| <- sp at return trampoline
3: |r11 | <- regs->sp at syscall
2: |rcx |
1: |rax | <- ri->stack
0: |ret-addr|
(Note: The lower the line, the larger the address.)
Thus, we can check the stack address by (regs->sp + 16 == ri->stack).
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Oleg Nesterov @ 2024-04-06 17:55 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Jiri Olsa, Andrii Nakryiko, Steven Rostedt, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, bpf, Song Liu, Yonghong Song, John Fastabend,
Peter Zijlstra, Thomas Gleixner, Borislav Petkov (AMD), x86,
linux-api
In-Reply-To: <20240406120536.57374198f3f45e809d7e4efa@kernel.org>
On 04/06, Masami Hiramatsu wrote:
>
> On Fri, 5 Apr 2024 13:02:30 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > With or without this patch userpace can also do
> >
> > foo() { <-- retprobe1
> > bar() {
> > jump to xol_area
> > }
> > }
> >
> > handle_trampoline() will handle retprobe1.
>
> This is OK because the execution path has been changed to trampoline,
Agreed, in this case the misuse is more clear. But please see below.
> but the above will continue running bar() after sys_uretprobe().
... and most probably crash
> > sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> > and return -EINVAL if this addr is not valid. But why?
>
> Because sigreturn() never returns, but sys_uretprobe() will return.
You mean, sys_uretprobe() returns to the next insn after syscall.
Almost certainly yes, but this is not necessarily true. If one of consumers
changes regs->sp sys_uretprobe() "returns" to another location, just like
sys_rt_sigreturn().
That said.
Masami, it is not that I am trying to prove that you are "wrong" ;) No.
I see your points even if I am biased, I understand that my objections are
not 100% "fair".
I am just trying to explain why, rightly or not, I care much less about the
abuse of sys_uretprobe().
Thanks!
Oleg.
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Masami Hiramatsu @ 2024-04-06 3:05 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240405110230.GA22839@redhat.com>
On Fri, 5 Apr 2024 13:02:30 +0200
Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/05, Jiri Olsa wrote:
> >
> > On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> > >
> > > I think this expects setjmp/longjmp as below
> > >
> > > foo() { <- retprobe1
> > > setjmp()
> > > bar() { <- retprobe2
> > > longjmp()
> > > }
> > > } <- return to trampoline
> > >
> > > In this case, we need to skip retprobe2's instance.
>
> Yes,
>
> > > My concern is, if we can not find appropriate return instance, what happen?
> > > e.g.
> > >
> > > foo() { <-- retprobe1
> > > bar() { # sp is decremented
> > > sys_uretprobe() <-- ??
> > > }
> > > }
> > >
> > > It seems sys_uretprobe() will handle retprobe1 at that point instead of
> > > SIGILL.
> >
> > yes, and I think it's fine, you get the consumer called in wrong place,
> > but it's your fault and kernel won't crash
>
> Agreed.
>
> With or without this patch userpace can also do
>
> foo() { <-- retprobe1
> bar() {
> jump to xol_area
> }
> }
>
> handle_trampoline() will handle retprobe1.
This is OK because the execution path has been changed to trampoline, but
the above will continue running bar() after sys_uretprobe().
>
> > this can be fixed by checking the syscall is called from the trampoline
> > and prevent handle_trampoline call if it's not
>
> Yes, but I still do not think this makes a lot of sense. But I won't argue.
>
> And what should sys_uretprobe() do if it is not called from the trampoline?
> I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
>
> I agree very much with Andrii,
>
> sigreturn() exists only to allow the implementation of signal handlers. It should never be
> called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
> the architecture.
>
> this is how sys_uretprobe() should be treated/documented.
OK.
>
> sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
> and return -EINVAL if this addr is not valid. But why?
Because sigreturn() never returns, but sys_uretprobe() will return.
If it changes the regs->ip and directly returns to the original return address,
I think it is natural to send SIGILL.
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* RE: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-06 0:04 UTC (permalink / raw)
To: Jonathan Cameron, linux-cxl
Cc: Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, Williams, Dan J, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240321174423.00007e0d@Huawei.com>
Jonathan Cameron wrote:
> Hi All,
>
> This is has come up in a number of discussions both on list and in private,
> so I wanted to lay out a potential set of rules when deciding whether or not
> to provide a user space interface for a particular feature of CXL Fabric
> Management. The intent is to drive discussion, not to simply tell people
> a set of rules. I've brought this to the public lists as it's a Linux kernel
> policy discussion, not a standards one.
>
> Whilst I'm writing the RFC this my attempt to summarize a possible
> position rather than necessarily being my personal view.
>
> It's a straw man - shoot at it!
>
> Not everyone in this discussion is familiar with relevant kernel or CXL concepts
> so I've provided more info than I normally would.
Thanks for writing this up Jonathan!
[..]
> 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> ==========================================================================
>
> (This would just be a kernel option that we'd advise normal server
> distributions not to turn on. Would be enabled by openBMC etc)
>
> This is fine - there is some work to do, but the switch-cci PCI driver
> will hopefully be ready for upstream merge soon. There is no filtering of
> accesses. Think of this as similar to all the damage you can do via
> MCTP from a BMC. Similarly it is likely that much of the complexity
> of the actual commands will be left to user space tooling:
> https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
>
> Whether Kconfig help text is strong enough to ensure this only gets
> enabled for BMC targeted distros is an open question we can address
> alongside an updated patch set.
It is not clear to me that this material makes sense to house in
drivers/ vs tools/ or even out-of-tree just for maintenance burden
relief of keeping the universes separated. What does the Linux kernel
project get out of carrying this in mainline alongside the inband code?
I do think the mailbox refactoring to support non-CXL use cases is
interesting, but only so far as refactoring is consumed for inband use
cases like RAS API.
> (On to the one that the "debate" is about)
>
> 3) Unfiltered user space use of mailbox for Fabric Management - Distro kernels
> =============================================================================
> (General purpose Linux Server Distro (Redhat, Suse etc))
>
> This is equivalent of RAW command support on CXL Type 3 memory devices.
> You can enable those in a distro kernel build despite the scary config
> help text, but if you use it the kernel is tainted. The result
> of the taint is to add a flag to bug reports and print a big message to say
> that you've used a feature that might result in you shooting yourself
> in the foot.
>
> The taint is there because software is not at first written to deal with
> everything that can happen smoothly (e.g. surprise removal) It's hard
> to survive some of these events, so is never on the initial feature list
> for any bus, so this flag is just to indicate we have entered a world
> where almost all bets are off wrt to stability. We might not know what
> a command does so we can't assess the impact (and no one trusts vendor
> commands to report affects right in the Command Effects Log - which
> in theory tells you if a command can result problems).
That is a secondary reason that the taint is there. Yes, it helps
upstream not waste their time on bug reports from proprietary use cases,
but the effect of that is to make "raw" command mode unattractive for
deploying solutions at scale. It clarifies that this interface is a
debug-tool that enterprise environment need not worry about.
The more salient reason for the taint, speaking only for myself as a
Linux kernel community member not for $employer, is to encourage open
collaboration. Take firmware-update for example that is a standard
command with known side effects that is inaccessible via the ioctl()
path. It is placed behind an ABI that is easier to maintain and reason
about. Everyone has the firmware update tool if they have the 'cat'
command. Distros appreciate the fact that they do not need ship yet
another vendor device-update tool, vendors get free tooling and end
users also appreciate one flow for all devices.
As I alluded here [1], I am not against innovation outside of the
specification, but it needs to be open, and it needs to plausibly become
if not a de jure standard at least a de facto standard.
[1]: https://lore.kernel.org/all/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> (a r3.1 feature) but, as I understand it, these are intended for a
> host to configure and should not have side effects on other hosts?
> My working assumption is that the kernel driver stack will handle
> these (once we catch up with the current feature backlog!) Currently
> we have no visibility of what the OS driver stack for a fabrics will
> actually look like - the spec is just the starting point for that.
> (patches welcome ;)
>
> The various CXL upstream developers and maintainers may have
> differing views of course, but my current understanding is we want
> to support 1 and 2, but are very resistant to 3!
1, yes, 2, need to see the patches, and agree on 3.
> General Notes
> =============
>
> One side aspect of why we really don't like unfiltered userspace access to any
> of these devices is that people start building non standard hacks in and we
> lose the ecosystem advantages. Forcing a considered discussion + patches
> to let a particular command be supported, drives standardization.
Like I said above, I think this is not a side aspect. It is fundamental
to the viability Linux as a project. This project only works because
organizations with competing goals realize they need some common
infrastructure and that there is little to be gained by competing on the
commons.
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> provides some history on vendor specific extensions and why in general we
> won't support them upstream.
Oh, you linked my writeup... I will leave the commentary I added here in case
restating it helps.
> To address another question raised in an earlier discussion:
> Putting these Fabric Management interfaces behind guard rails of some type
> (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> of non standard interfaces, because we will be even less likely to accept
> those upstream!
>
> If anyone needs more details on any aspect of this please ask.
> There are a lot of things involved and I've only tried to give a fairly
> minimal illustration to drive the discussion. I may well have missed
> something crucial.
You captured it well, and this is open source so I may have missed
something crucial as well.
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Oleg Nesterov @ 2024-04-05 11:02 UTC (permalink / raw)
To: Jiri Olsa
Cc: Masami Hiramatsu, Andrii Nakryiko, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <Zg-8r63tPSkuhN7p@krava>
On 04/05, Jiri Olsa wrote:
>
> On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> >
> > I think this expects setjmp/longjmp as below
> >
> > foo() { <- retprobe1
> > setjmp()
> > bar() { <- retprobe2
> > longjmp()
> > }
> > } <- return to trampoline
> >
> > In this case, we need to skip retprobe2's instance.
Yes,
> > My concern is, if we can not find appropriate return instance, what happen?
> > e.g.
> >
> > foo() { <-- retprobe1
> > bar() { # sp is decremented
> > sys_uretprobe() <-- ??
> > }
> > }
> >
> > It seems sys_uretprobe() will handle retprobe1 at that point instead of
> > SIGILL.
>
> yes, and I think it's fine, you get the consumer called in wrong place,
> but it's your fault and kernel won't crash
Agreed.
With or without this patch userpace can also do
foo() { <-- retprobe1
bar() {
jump to xol_area
}
}
handle_trampoline() will handle retprobe1.
> this can be fixed by checking the syscall is called from the trampoline
> and prevent handle_trampoline call if it's not
Yes, but I still do not think this makes a lot of sense. But I won't argue.
And what should sys_uretprobe() do if it is not called from the trampoline?
I'd prefer force_sig(SIGILL) to punish the abuser ;) OK, OK, EINVAL.
I agree very much with Andrii,
sigreturn() exists only to allow the implementation of signal handlers. It should never be
called directly. Details of the arguments (if any) passed to sigreturn() vary depending on
the architecture.
this is how sys_uretprobe() should be treated/documented.
sigreturn() can be "improved" too. Say, it could validate sigcontext->ip
and return -EINVAL if this addr is not valid. But why?
Oleg.
^ permalink raw reply
* Re: [PATCHv2 1/3] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-04-05 8:56 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Oleg Nesterov, Andrii Nakryiko, Jiri Olsa, Steven Rostedt,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, bpf, Song Liu, Yonghong Song,
John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), x86, linux-api
In-Reply-To: <20240405102203.825c4a2e9d1c2be5b2bffe96@kernel.org>
On Fri, Apr 05, 2024 at 10:22:03AM +0900, Masami Hiramatsu wrote:
> On Thu, 4 Apr 2024 18:11:09 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 04/05, Masami Hiramatsu wrote:
> > >
> > > Can we make this syscall and uprobe behavior clearer? As you said, if
> > > the application use sigreturn or longjump, it may skip returns and
> > > shadow stack entries are left in the kernel. In such cases, can uretprobe
> > > detect it properly, or just crash the process (or process runs wrongly)?
> >
> > Please see the comment in handle_trampoline(), it tries to detect this case.
> > This patch should not make any difference.
>
> I think you mean this loop will skip and discard the stacked return_instance
> to find the valid one.
>
> ----
> do {
> /*
> * We should throw out the frames invalidated by longjmp().
> * If this chain is valid, then the next one should be alive
> * or NULL; the latter case means that nobody but ri->func
> * could hit this trampoline on return. TODO: sigaltstack().
> */
> next = find_next_ret_chain(ri);
> valid = !next || arch_uretprobe_is_alive(next, RP_CHECK_RET, regs);
>
> instruction_pointer_set(regs, ri->orig_ret_vaddr);
> do {
> if (valid)
> handle_uretprobe_chain(ri, regs);
> ri = free_ret_instance(ri);
> utask->depth--;
> } while (ri != next);
> } while (!valid);
> ----
>
> I think this expects setjmp/longjmp as below
>
> foo() { <- retprobe1
> setjmp()
> bar() { <- retprobe2
> longjmp()
> }
> } <- return to trampoline
>
> In this case, we need to skip retprobe2's instance.
> My concern is, if we can not find appropriate return instance, what happen?
> e.g.
>
> foo() { <-- retprobe1
> bar() { # sp is decremented
> sys_uretprobe() <-- ??
> }
> }
>
> It seems sys_uretprobe() will handle retprobe1 at that point instead of
> SIGILL.
yes, and I think it's fine, you get the consumer called in wrong place,
but it's your fault and kernel won't crash
this can be fixed by checking the syscall is called from the trampoline
and prevent handle_trampoline call if it's not
>
> Can we avoid this with below strict check?
>
> if (ri->stack != regs->sp + expected_offset)
> goto sigill;
hm the current uprobe 'alive' check makes sure the return_instance is above
or at the same stack address, not sure we can match it exactly, need to think
about that more
>
> expected_offset should be 16 (push * 3 - ret) on x64 if we ri->stack is the
> regs->sp right after call.
the syscall trampoline already updates the regs->sp before calling
handle_trampoline
regs->sp += sizeof(r11_cx_ax);
jirka
^ 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