On Wed, Nov 27, 2019 at 6:11 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
> I posted this a few weeks back, took another look at it and refined it a
> bit. I'd like some input on the viability of this approach.
>
> A new signalfd setup flag is added, SFD_TASK. This is only valid if used
> with SFD_CLOEXEC. If set, the task setting up the signalfd descriptor is
> remembered in the signalfd context, and will be the one we use for
> checking signals in the poll/read handlers in signalfd.
>
> This is needed to make signalfd useful with io_uring and aio, of which
> the former in particular has my interest.
>
> I _think_ this is sane. To prevent the case of a task clearing O_CLOEXEC
> on the signalfd descriptor, forking, and then exiting, we grab a
> reference to the task when we assign it. If that original task exits, we
> catch it in signalfd_flush() and ensure waiters are woken up.
Mh... that's not really reliable, because you only get ->flush() from
the last exiting thread (or more precisely, the last exiting task that
shares the files_struct).
What is your goal here? To have a reference to a task without keeping
the entire task_struct around in memory if someone leaks the signalfd
to another process - basically like a weak pointer? If so, you could
store a refcounted reference to "struct pid" instead of a refcounted
reference to the task_struct, and then do the lookup of the
task_struct on ->poll and ->read (similar to what procfs does).
In other words:
> diff --git a/fs/signalfd.c b/fs/signalfd.c
> index 44b6845b071c..4bbdab9438c1 100644
> --- a/fs/signalfd.c
> +++ b/fs/signalfd.c
> @@ -50,28 +50,62 @@ void signalfd_cleanup(struct sighand_struct *sighand)
>
> struct signalfd_ctx {
> sigset_t sigmask;
> + struct task_struct *task;
Turn this into "struct pid *task_pid".
> +static int signalfd_flush(struct file *file, void *data)
> +{
> + struct signalfd_ctx *ctx = file->private_data;
> + struct task_struct *tsk = ctx->task;
> +
> + if (tsk == current) {
> + ctx->task = NULL;
> + wake_up(&tsk->sighand->signalfd_wqh);
> + put_task_struct(tsk);
> + }
> +
> + return 0;
> +}
Get rid of this.
> +static struct task_struct *signalfd_get_task(struct signalfd_ctx *ctx)
> +{
> + struct task_struct *tsk = ctx->task ?: current;
> +
> + get_task_struct(tsk);
> + return tsk;
> +}
Replace this with something like:
if (ctx->task_pid)
return get_pid_task(ctx->task_pid, PIDTYPE_PID); /* will return
NULL if the task is gone */
else
return get_task_struct(current);
and add NULL checks to the places that call this.
> @@ -167,10 +201,11 @@ static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, kernel_siginfo_t *info
> int nonblock)
> {
> ssize_t ret;
> + struct task_struct *tsk = signalfd_get_task(ctx);
(Here we could even optimize away the refcounting using RCU if we
wanted to, since unlike in the ->poll handler, we don't need to be
able to block.)
> if (ufd == -1) {
> - ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> return -ENOMEM;
>
> ctx->sigmask = *mask;
> + if (flags & SFD_TASK) {
> + ctx->task = current;
> + get_task_struct(ctx->task);
> + }
and here do "ctx->task_pid = get_task_pid(current, PIDTYPE_PID)"
