linux-kernel - Re: [patch 2/5] signalfd v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070308215653.GA6744@tv-sign.ru>
Date:	Fri, 9 Mar 2007 00:56:53 +0300
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Davide Libenzi <davidel@...ilserver.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Avi Kivity <avi@...o.co.il>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch 2/5] signalfd v2 - signalfd core ...

On 03/08, Davide Libenzi wrote:
>
> On Thu, 8 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;
> > > +	struct signalfd_sq *sq;
> > > +
> > > +	list_for_each(pos, &sighand->sfdlist) {
> > > +		ctx = list_entry(pos, struct signalfd_ctx, lnk);
> > > +		/*
> > > +		 * 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.
> > > +		 */
> > > +		if (sig < 0)
> > > +			__wake_up_locked(&ctx->wqh, TASK_UNINTERRUPTIBLE | TASK_INTERRUPTIBLE);
> > > +		else if (sigismember(&ctx->sigmask, sig) &&
> > > +			 (sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))) {
> > > +			sigaddset(&ctx->pending, sig);
> > 
> > I don't understand the "(sig >= SIGRTMIN || !sigismember(&ctx->pending, sig))"
> > check. This mimics the LEGACY_QUEUE() check, but seems strange. The signal may
> > be pending in ctx->pending just because it was not signalfd_fetchsig()ed, yes?
> 
> Logic is, if it's not an RT signal, queue only one, otherwise multiple.
> The bit on the ->pending mask is clealer only when the queue slot becomes empty.

Yes, I see what the code does, but I don't undestand why. For example, SIGCHLD was
delivered to the process _and_ handled several times, then sys_signalfd_dequeue()
comes and finds only one siginfo. Isn't this strange?

> > > @@ -780,6 +785,11 @@
> > >  	BUG_ON(!irqs_disabled());
> > >  	assert_spin_locked(&t->sighand->siglock);
> > >  
> > > +	/*
> > > +	 * Deliver the signal to listening signalfds ...
> > > +	 */
> > > +	signalfd_notify(t->sighand, sig, info);
> > > +
> > >  	/* Short-circuit ignored signals.  */
> > >  	if (sig_ignored(t, sig))
> > >  		goto out;
> > > @@ -968,6 +978,11 @@
> > >  	assert_spin_locked(&p->sighand->siglock);
> > >  	handle_stop_signal(sig, p);
> > >  
> > > +	/*
> > > +	 * Deliver the signal to listening signalfds ...
> > > +	 */
> > > +	signalfd_notify(p->sighand, sig, info);
> > > +
> > >  	/* Short-circuit ignored signals.  */
> > >  	if (sig_ignored(p, sig))
> > >  		return ret;
> > 
> > It is strange that we are doing signalfd_notify() even if the signal is ignored.
> > Isn't it better to shift signalfd_notify() into send_signal() ? This way we do
> > not need the special check in signalfd_deliver() above.
> 
> The two trasports can rely on different masks. The signalfd_notify() does 
> not even go in signalfd_deliver() if no signalfds are attached to the 
> sighand.

Sorry, I don't understand. The masks are different, yes, but ->sighand is the
same? How this can make any difference for "if no signalfds are attached" ?

OK. What is the purpose of signalfd? Should it record the signals which were
sent to the process, or only those which were delivered? The latter looks
more natural for me. But inn any case, I don't see a reason to check ->pending
mask in signalfd_deliver().

BTW, sys_signalfd_dequeue() re-queues signalfd_sq if copy_siginfo_to_user()
fails. Why not -EFAULT?

Also. A malicious user can eat all memory, signalfd_deliver()->kmem_cache_alloc()
doesn't check any limits.

> > Also, this patch doesn't take send_sigqueue/send_group_sigqueue into account.
> 
> I added that too. but I noticed something strange, dunno if intentional or 
> not. In send_sigqueue and send_group_sigqueue, the check for the 
> timer-special and the ignored is inverted. This lead to two different 
> behaviours. Is there a reason for that?

I was wondering about that too.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/