Re: [PATCH 1/2] rv/nomiss: fix task storage lifetime

[Gabriele Monaco <gmonaco@redhat.com>]

On Wed, 2026-07-15 at 14:00 +0800, Li Qiang wrote:
> The nomiss monitor stores a pointer to a task's embedded deadline entity
> in per-PID object storage. The exit handler only removed that storage when
> the task was still SCHED_DEADLINE. A task that changed to another policy
> before exiting left its entry behind. Once the PID was reused, the new task
> could reuse the old entry and dereference the freed deadline entity.

Yes, this is actually an issue, but wouldn't it be solved by just deleting the
storage as soon as a task changes its policy out of DEADLINE ?

We are currently only resetting the monitor (so timers won't fire), but deleting
would probably be more appropriate.

Then there's the issue we are tracking the syscall entry, so we don't know if
that is going to succeed, a more robust solution would be to add a tracepoint
inside the actual __sched_setscheduler() (but maintainers may not want that) or
tracking the context (old policy) inside sys_enter and commit the result in
sys_exit only if it succeeds.

> Remove storage unconditionally at sched_process_exit. Serialize creation
> and destruction with locks sharded by PID, and do not create storage once
> PF_EXITING is set. Each shard tracks both stored entries and in-flight
> creators, allowing exits for shards without storage to avoid locking and
> hash lookups while preserving the create/exit ordering.

I don't get why we need this complex machinery. What races are you trying to
solve with this locks? Concurrent syscalls and newtask/exit can this even
happen?

Thanks,
Gabriele

> 
> Fixes: b133207deb72 ("rv: Add nomiss deadline monitor")
> Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
> ---
>  kernel/trace/rv/monitors/deadline/deadline.h | 86 +++++++++++++++++++-
>  kernel/trace/rv/monitors/nomiss/nomiss.c     |  2 +-
>  2 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/rv/monitors/deadline/deadline.h
> b/kernel/trace/rv/monitors/deadline/deadline.h
> index 78fca873d61e..b92480e79807 100644
> --- a/kernel/trace/rv/monitors/deadline/deadline.h
> +++ b/kernel/trace/rv/monitors/deadline/deadline.h
> @@ -1,6 +1,8 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  
>  #include <linux/kernel.h>
> +#include <linux/hash.h>
> +#include <linux/spinlock.h>
>  #include <linux/uaccess.h>
>  #include <linux/sched/deadline.h>
>  #include <asm/syscall.h>
> @@ -148,6 +150,37 @@ static inline struct sched_dl_entity *get_server(struct
> task_struct *tsk, u8 typ
>  	return NULL;
>  }
>  
> +#define DEADLINE_STORAGE_LOCK_BITS	6
> +#define DEADLINE_STORAGE_LOCKS		BIT(DEADLINE_STORAGE_LOCK_BITS)
> +
> +/*
> + * A shard tracks both entries and creators which have not yet checked
> + * PF_EXITING. This lets unrelated exits avoid taking a storage lock while
> + * preserving the create/exit ordering.
> + */
> +struct deadline_storage_shard {
> +	raw_spinlock_t lock;
> +	atomic_t users;
> +} ____cacheline_aligned_in_smp;
> +
> +static struct deadline_storage_shard
> +	deadline_storage_shards[DEADLINE_STORAGE_LOCKS];
> +
> +static inline struct deadline_storage_shard *deadline_storage_shard(int pid)
> +{
> +	return &deadline_storage_shards[hash_32(pid,
> DEADLINE_STORAGE_LOCK_BITS)];
> +}
> +
> +static void deadline_storage_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < DEADLINE_STORAGE_LOCKS; i++) {
> +		raw_spin_lock_init(&deadline_storage_shards[i].lock);
> +		atomic_set(&deadline_storage_shards[i].users, 0);
> +	}
> +}
> +
>  /*
>   * Initialise monitors for all tasks and pre-allocate the storage for
> servers.
>   * This is necessary since we don't have access to the servers here and
> @@ -159,6 +192,8 @@ static inline int init_storage(bool skip_tasks)
>  	struct task_struct *g, *p;
>  	int cpu;
>  
> +	deadline_storage_init();
> +
>  	for_each_possible_cpu(cpu) {
>  		if (!da_create_empty_storage(fair_server_id(cpu)))
>  			goto fail;
> @@ -177,6 +212,7 @@ static inline int init_storage(bool skip_tasks)
>  				read_unlock(&tasklist_lock);
>  				goto fail;
>  			}
> +			atomic_inc(&deadline_storage_shard(p->pid)->users);
>  		}
>  	}
>  	read_unlock(&tasklist_lock);
> @@ -187,17 +223,61 @@ static inline int init_storage(bool skip_tasks)
>  	return -ENOMEM;
>  }
>  
> +static void deadline_create_task_storage(struct task_struct *task)
> +{
> +	struct deadline_storage_shard *shard = deadline_storage_shard(task-
> >pid);
> +	bool created = false;
> +
> +	/*
> +	 * The temporary reference makes an in-flight creator visible to
> exit.
> +	 * If storage is created, it becomes the reference owned by that
> entry.
> +	 */
> +	atomic_inc(&shard->users);
> +	/* Pair with exit before it tests users without taking the shard
> lock. */
> +	smp_mb__after_atomic();
> +
> +	raw_spin_lock(&shard->lock);
> +	if (!(READ_ONCE(task->flags) & PF_EXITING)) {
> +		guard(rcu)();
> +		if (!da_get_monitor(EXPAND_ID_TASK(task)) &&
> +		    da_create_storage(EXPAND_ID_TASK(task), NULL))
> +			created = true;
> +	}
> +	raw_spin_unlock(&shard->lock);
> +
> +	if (!created)
> +		atomic_dec(&shard->users);
> +}
> +
> +static void deadline_destroy_task_storage(struct task_struct *task)
> +{
> +	struct deadline_storage_shard *shard = deadline_storage_shard(task-
> >pid);
> +
> +	/* Pair with the creator barrier before taking the no-storage fast
> path. */
> +	smp_mb();
> +	if (!atomic_read(&shard->users))
> +		return;
> +
> +	raw_spin_lock(&shard->lock);
> +	guard(rcu)();
> +	if (da_get_monitor(task->pid, NULL)) {
> +		/* A task may leave SCHED_DEADLINE before exiting. */
> +		da_destroy_storage(task->pid);
> +		atomic_dec(&shard->users);
> +	}
> +	raw_spin_unlock(&shard->lock);
> +}
> +
>  static void __maybe_unused handle_newtask(void *data, struct task_struct
> *task, u64 flags)
>  {
>  	/* Might be superfluous as tasks are not started with this policy..
> */
>  	if (task->policy == SCHED_DEADLINE)
> -		da_create_storage(EXPAND_ID_TASK(task), NULL);
> +		deadline_create_task_storage(task);
>  }
>  
>  static void __maybe_unused handle_exit(void *data, struct task_struct *p,
> bool group_dead)
>  {
> -	if (p->policy == SCHED_DEADLINE)
> -		da_destroy_storage(get_entity_id(&p->dl, DL_TASK, DL_TASK));
> +	deadline_destroy_task_storage(p);
>  }
>  
>  #endif
> diff --git a/kernel/trace/rv/monitors/nomiss/nomiss.c
> b/kernel/trace/rv/monitors/nomiss/nomiss.c
> index 8ead8783c29f..2e5ee681505b 100644
> --- a/kernel/trace/rv/monitors/nomiss/nomiss.c
> +++ b/kernel/trace/rv/monitors/nomiss/nomiss.c
> @@ -214,7 +214,7 @@ static void handle_sys_enter(void *data, struct pt_regs
> *regs, long id)
>  	if (p->policy == SCHED_DEADLINE)
>  		da_reset(EXPAND_ID_TASK(p));
>  	else if (new_policy == SCHED_DEADLINE)
> -		da_create_or_get(EXPAND_ID_TASK(p));
> +		deadline_create_task_storage(p);
>  }
>  
>  static void handle_sched_wakeup(void *data, struct task_struct *tsk)