Message-ID: <20090818114533.GA32289@redhat.com>
Date:	Tue, 18 Aug 2009 13:45:33 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	stephane eranian <eranian@...glemail.com>
Cc:	Jamie Lokier <jamie@...reable.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>, mingo@...e.hu,
	linux-kernel@...r.kernel.org, tglx@...utronix.de,
	robert.richter@....com, paulus@...ba.org, andi@...stfloor.org,
	mpjohn@...ibm.com, cel@...ibm.com, cjashfor@...ibm.com,
	mucci@...s.utk.edu, terpstra@...s.utk.edu,
	perfmon2-devel@...ts.sourceforge.net, mtk.manpages@...glemail.com,
	roland@...hat.com
Subject: Re: F_SETOWN_TID: F_SETOWN was thread-specific for a while

On 08/18, stephane eranian wrote:
>
> On Mon, Aug 17, 2009 at 7:16 PM, Oleg Nesterov<oleg@...hat.com> wrote:
> > Sorry for late reply...
> >
> > On 08/10, stephane eranian wrote:
> >>
> >> On Mon, Aug 10, 2009 at 7:03 PM, Oleg Nesterov<oleg@...hat.com> wrote:
> >> >
> >> > Not sure if it is safe to change the historical behaviour.
> >> >
> >> Don't need to change it.
> >
> > Good,
> >
> >> But for SIGIO, if you see SA_SIGINFO, then pass the si_fd.
> >
> > But this means we do change the behaviour ;) Confused.
> >
> I meant do not remove F_SETSIG and its side-effect on si_fd.

Ah, now I see what you meant.

> > In any case. We should not look at SA_SIGINFO at all. If sys_sigaction() was
> > called without SA_SIGINFO, then it doesn'matter if we send SEND_SIG_PRIV or
> > siginfo_t with the correct si_fd/etc.
> >
> What's the official role of SA_SIGINFO? Pass a siginfo struct?
>
> Does POSIX describe the rules governing the content of si_fd?
> Or is si_fd a Linux-ony extension in which case it goes with F_SETSIG.

Not sure I understand your concern...

OK. You suggest to pass siginfo_t with .si_fd/etc when we detect SA_SIGINFO.

But, in that case we can _always_ pass siginfo_t, regardless of SA_SIGINFO.
If the task has a signal handler and sigaction() was called without
SA_SIGINFO, then the handler must not look into *info (the second arg of
sigaction->sa_sigaction). And in fact, __setup_rt_frame() doesn't even
copy the info to the user-space:

	if (ka->sa.sa_flags & SA_SIGINFO) {
		if (copy_siginfo_to_user(&frame->info, info))
			return -EFAULT;
	}

OK? Or I missed something?

> > And again, this is even documented. The change is trivial but user-space
> > visible, it may confuse the (stupid) app which uses SIGIO + SA_SIGINFO
> > without F_SETSIG.
> >
> That would be an app that uses SIGINFO and fiddles with si_fd which has no
> defined content. What kind of app would that be?

The stupid app. But it is always unsafe to make the user-visible changes
without good reasons. Even when we fix the bug (and the current code is not
buggy) sometimes we have "this patch breaks my app or test-case!" reports.
If nothing else, we can break the test-case which simply does

	void sigio_handler(int sig, siginfo_t *info, void *u)
	{
		assert(info->si_code == 0 && info->si_code = 0x80);
	}

Once again: this is _documented_ !

And we can't set .si_fd = fd whithout changing .si_code, this will break
copy_siginfo_to_user().

Or. Suppose that some app does:

	void io_handler(int sig, siginfo_t *info, void *u)
	{
		if ((info->si_code & __SI_MASK) != SI_POLL) {
			// RT signal failed! sig MUST be == SIGIO
			recover();
		} else {
			do_something(info->si_fd);
		}
	}

	int main(void)
	{
		sigaction(SIGRTMIN, { SA_SIGINFO, io_handler });
		sigaction(SIGIO,    { SA_SIGINFO, io_handler });
		...
	}

This is correct. But if we change the current behaviour, this app won't
be able to detect the overflow.

> It would seem natural that in the siginfo passed to the handler of SIGIO, the
> file descriptor be passed by default. That is all I am trying to say here.

Completely agreed! I was always puzzled by send_sigio_to_task(). I was never
able to understand why it looks so strange.

So, I think it should be

	static void send_sigio_to_task(struct task_struct *p,
				       struct fown_struct *fown,
				       int fd,
				       int reason)
	{
		siginfo_t si;
		/*
		 * F_SETSIG can change ->signum lockless in parallel, make
		 * sure we read it once and use the same value throughout.
		 */
		int signum = ACCESS_ONCE(fown->signum) ?: SIGIO;

		if (!sigio_perm(p, fown, signum))
			return;

		si.si_signo = signum;
		si.si_errno = 0;
		si.si_code  = reason;
		si.si_fd    = fd;
		/* Make sure we are called with one of the POLL_*
		   reasons, otherwise we could leak kernel stack into
		   userspace.  */
		BUG_ON((reason & __SI_MASK) != __SI_POLL);
		if (reason - POLL_IN >= NSIGPOLL)
			si.si_band  = ~0L;
		else
			si.si_band = band_table[reason - POLL_IN];

		/* Failure to queue an rt signal must be reported as SIGIO */
		if (!group_send_sig_info(signum, &si, p))
			group_send_sig_info(SIGIO, SEND_SIG_PRIV, p);
	}

(except it should be on top of fcntl-add-f_etown_ex.patch).
This way, at least we don't break the "detect RT signal failed" above.

What do you think?

But let me repeat: I just can't convince myself we have a good reason
to change the strange, but carefully documented behaviour.

In case you agree with the code above, I can send the patch. But only
if I have a "good" changelog from you + your Signed-of-by in advance ;)

Otherwise, please feel free to send this/similar change yourself.

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/

Powered by blists - more mailing lists