On Sat, 10 Mar 2007, Oleg Nesterov wrote:
> Davide Libenzi wrote:
> >
> > +int signalfd_deliver(struct sighand_struct *sighand, int sig,
> > + struct siginfo *info)
> > +{
> > + int nsig = 0;
> > + struct list_head *pos;
> > + struct signalfd_ctx *ctx;
> > +
> > + list_for_each(pos, &sighand->sfdlist) {
> > + ctx = list_entry(pos, struct signalfd_ctx, lnk);
>
> list_for_each_entry()
Will do, thx!
> > + /*
> > + * We use a negative signal value as a way to broadcast that the
> > + * sighand has been orphaned, so that we can notify all the
> > + * listeners about this. Remeber the ctx->sigmask is inverted,
> > + * so if the user is interested in a signal, that corresponding
> > + * bit will be zero.
> > + */
> > + if (sig < 0 || !sigismember(&ctx->sigmask, sig)) {
> > + __wake_up_locked(&ctx->wqh,
> > + TASK_UNINTERRUPTIBLE |
> > TASK_INTERRUPTIBLE);
>
> wake_up_locked(&ctx->wqh)
Yeah, will do.
> > + ctx->sighand = current->sighand;
> > + atomic_inc(&ctx->sighand->count);
>
> I personally don't like this. de_thread() was/is the source of numerous
> problems, and this patch adds yet another subtle dependency. The usage of
> "private" __cleanup_sighand() is not good per se, imho.
This we agree ...
> Also, this is not so flexible, we can't take S_ISUID into account. It seems
> logical to preserve ctx after a "normal" exec.
This, not really. I'm not sure we want to leak this out of an exec.
> > + spin_lock_irq(&ctx->sighand->siglock);
> > + ctx->sigmask = sigmask;
> > + spin_unlock_irq(&ctx->sighand->siglock);
> > + wake_up(&ctx->wqh);
>
> Race with signalfd_read()->__add_wait_queue().
Ack.
> > + if (sighand != ctx->tsk->sighand || ctx->tsk->signal == NULL ||
>
> We don't need "ctx->tsk->signal == NULL". tsk->signal == NULL (when checked
> under ->siglock) implies tsk->sighand == NULL. This is covered by the first
> "sighand != ctx->tsk->sighand" check.
Extra check, due to rely less on the exit_signal code.
> > + __remove_wait_queue(&ctx->wqh, &wait);
> > + set_current_state(TASK_RUNNING);
>
> We don't need mb() here.
Ack.
> > +void signal_fill_info(struct siginfo *dinfo, int sig, struct siginfo
> > *sinfo)
> > +{
> > + switch ((unsigned long) sinfo) {
> > + case (unsigned long) SEND_SIG_NOINFO:
> > + dinfo->si_signo = sig;
> > + dinfo->si_errno = 0;
> > + dinfo->si_code = SI_USER;
> > + dinfo->si_pid = current->pid;
> > + dinfo->si_uid = current->uid;
> > + break;
> > + case (unsigned long) SEND_SIG_PRIV:
> > + dinfo->si_signo = sig;
> > + dinfo->si_errno = 0;
> > + dinfo->si_code = SI_KERNEL;
> > + dinfo->si_pid = 0;
> > + dinfo->si_uid = 0;
> > + break;
> > + default:
> > + copy_siginfo(dinfo, sinfo);
> > + }
> > +}
>
> This change seems unneeded?
Yes, it leaked out from the version of signalfd that had its own queue.
> I'd suggest to remove signalfd_ctx->sighand. de_thread()/exit_signal() call
>
> signalfd_exit_task(struct sighand_struct *sighand)
> {
> list_for_each_entry_safe(ctx, sighand->sfdlist)
> if (ctx->tsk == current) {
> wake_up_locked(ctx->wqh);
> list_del_init(ctx->lnk);
> }
> }
>
> signalfd_read()/signalfd_poll use
>
> static struct sighand_struct *ctx_try_to_lock(struct signalfd_ctx *ctx,
> flags)
> {
> struct sighand_struct *ret;
>
> rcu_read_lock();
> ret = lock_task_sighand(ctx->task);
> rcu_read_unlock();
>
> if (ret && list_empty(ctx->lnk)) {
> unlock_task_sighand(ctx->task);
> ret = NULL;
> }
>
> return ret;
> }
>
> instead of "spin_lock_irq(ctx->sighand)" + "if (ctx->sighand !=
> ctx->tsk->sighand)".
>
> Possible?
>
> Note that signalfd_exit_task() is generic, could be used in another context,
> de_thread() can avoid the call if !suid.
I think it looks good to me. Will give it a spin today.
> How about CONFIG_SIGNALFD, btw?
Yes, I was already planning it.
> Davide, could you please cc me? I am not subscribed to lkml, noticed the new
> version by accident.
Will do, thx!
- Davide
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
