Re: [PATCH] signalfd: fill in ssi_int for posix timers and message queues

[Nathan Lynch <ntl@pobox.com>]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

Hello Davide,

On Sat, 2010-07-03 at 12:09 -0700, Davide Libenzi wrote:
> On Fri, 2 Jul 2010, Nathan Lynch wrote:
> 
> > If signalfd is used to consume a signal generated by a POSIX interval
> > timer or POSIX message queue, the ssi_int field does not reflect the
> > data (sigevent->sigev_value) supplied to timer_create(2) or
> > mq_notify(3).  (The ssi_ptr field, however, is filled in.)
> > 
> > This behavior differs from signalfd's treatment of sigqueue-generated
> > signals -- see the default case in signalfd_copyinfo.  It also gives
> > results that differ from the case when a signal is handled
> > conventionally via a sigaction-registered handler.
> > 
> > So, set signalfd_siginfo->ssi_int in the remaining cases (__SI_TIMER,
> > __SI_MESGQ) where ssi_ptr is set.
> > 
> > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> > ---
> >  fs/signalfd.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/signalfd.c b/fs/signalfd.c
> > index f329849..1c5a6ad 100644
> > --- a/fs/signalfd.c
> > +++ b/fs/signalfd.c
> > @@ -88,6 +88,7 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> >  		 err |= __put_user(kinfo->si_tid, &uinfo->ssi_tid);
> >  		 err |= __put_user(kinfo->si_overrun, &uinfo->ssi_overrun);
> >  		 err |= __put_user((long) kinfo->si_ptr, &uinfo->ssi_ptr);
> > +		 err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
> >  		break;
> >  	case __SI_POLL:
> >  		err |= __put_user(kinfo->si_band, &uinfo->ssi_band);
> > @@ -111,6 +112,7 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> >  		err |= __put_user(kinfo->si_pid, &uinfo->ssi_pid);
> >  		err |= __put_user(kinfo->si_uid, &uinfo->ssi_uid);
> >  		err |= __put_user((long) kinfo->si_ptr, &uinfo->ssi_ptr);
> > +		err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
> >  		break;
> >  	default:
> 
> I am fine with it, but I now noticed that signalfd_copyinfo() got out of 
> sync from copy_siginfo_to_user(), which should match.
> Do you mind aligning that too, as part of your patch?
> An adding a comment on the lines of the one in copy_siginfo_to_user() to 
> signalfd_copyinfo() too?

Sorry, I'm not sure I understand.  Are you saying that
copy_siginfo_to_user should have analogous lines added to assign to
si_int?  That's actually not necessary if I read the code correctly: in
struct siginfo, si_ptr and si_int are members of a sigval union, so
assigning to the former covers the latter.  signalfd must assign both
ssi_ptr and ssi_int since they occupy different locations in
signalfd_siginfo.

Perhaps the attached testcases make the problem (as I see it) more
clear?  The final assertion fails without this patch.


[-- Attachment #2: mq_notify-vs-signalfd.c --]
[-- Type: text/x-csrc, Size: 2679 bytes --]

#include <sys/signalfd.h>
#include <assert.h>
#include <errno.h>
#include <mqueue.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static int handler_run;
static const union sigval my_sigval = { .sival_int = 42, };

static const char msgbuf[] = "hello";

#define bail(msg) \
	do { perror(msg); exit(EXIT_FAILURE); } while (0);

static void handler(int sig, siginfo_t *si, void *notused)
{
	assert(si->si_code == SI_MESGQ);
	assert(si->si_ptr == my_sigval.sival_ptr); /* Succeeds */
	assert(si->si_int == my_sigval.sival_int); /* Succeeds */

	/* This should run only once */
	assert(handler_run == 0);
	handler_run++;
}

int main(int argc, char *argv[])
{
	struct signalfd_siginfo fdsi;
	struct sigaction newact;
	struct sigevent sev;
	struct mq_attr attr;
	sigset_t mask;
	size_t msglen;
	char *rcvbuf;
	mqd_t mqdes;
	ssize_t s;
	int sfd;

	if (argc != 2) {
		fprintf(stderr, "Usage: %s <mq-name>\n", argv[0]);
		exit(EXIT_FAILURE);
	}

	newact.sa_flags = SA_SIGINFO | SA_RESETHAND;
	newact.sa_sigaction = handler;
	sigemptyset(&newact.sa_mask);

	if (sigaction(SIGRTMIN, &newact, NULL) == -1)
		bail("sigaction");

	mqdes = mq_unlink(argv[1]);
	if (mqdes == -1 && errno != ENOENT)
		bail("mq_unlink");

	mqdes = mq_open(argv[1], (O_RDWR | O_CREAT), 0600, NULL);
	if (mqdes == (mqd_t) -1)
		bail("mq_open");

	memset(&sev, 0, sizeof(sev));
	sev.sigev_notify = SIGEV_SIGNAL;
	sev.sigev_signo = SIGRTMIN;
	sev.sigev_value = my_sigval;

	if (mq_notify(mqdes, &sev) == -1)
		bail("mq_notify");

	if (mq_send(mqdes, msgbuf, sizeof(msgbuf), 0) == -1)
		bail("mq_send");

	while (!handler_run)
		usleep(100000);

	if (mq_getattr(mqdes, &attr) == -1)
		bail("mq_getattr");

	msglen = attr.mq_msgsize;

	rcvbuf = malloc(msglen);
	if (!rcvbuf)
		bail("malloc");

	/* Empty the queue */
	if (mq_receive(mqdes, rcvbuf, msglen, NULL) != sizeof(msgbuf))
		bail("mq_receive");

	/* Inhibit default SIGRTMIN handling */
	sigemptyset(&mask);
	sigaddset(&mask, SIGRTMIN);
	if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
		bail("sigprocmask");

	/* Reregister the notifier and post another message */
	if (mq_notify(mqdes, &sev) == -1)
		bail("mq_notify");

	if (mq_send(mqdes, msgbuf, sizeof(msgbuf), 0) == -1)
		bail("mq_send");

	sfd = signalfd(-1, &mask, 0);
	if (sfd == -1)
		bail("signalfd");

	/* Handle the signal */
	s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
	if (s != sizeof(struct signalfd_siginfo))
		bail("read");

	assert(fdsi.ssi_code == SI_MESGQ);
	assert(fdsi.ssi_ptr ==
	       (unsigned long)my_sigval.sival_ptr); /* Succeeds */
	assert(fdsi.ssi_int == my_sigval.sival_int); /* Fails */

	exit(EXIT_SUCCESS);
}

[-- Attachment #3: timer-vs-signalfd.c --]
[-- Type: text/x-csrc, Size: 2228 bytes --]

#include <sys/signalfd.h>
#include <assert.h>
#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <signal.h>
#include <string.h>
#include <time.h>

static int handler_run;
static const union sigval my_sigval = { .sival_int = 42, };

#define bail(msg) \
	do { perror(msg); exit(EXIT_FAILURE); } while (0);

static void handler(int sig, siginfo_t *si, void *notused)
{
	assert(si->si_code == SI_TIMER);
	assert(si->si_ptr == my_sigval.sival_ptr); /* Succeeds */
	assert(si->si_int == my_sigval.sival_int); /* Succeeds */

	/* This should run only once */
	assert(handler_run == 0);
	handler_run++;
}

int main(void)
{
	struct signalfd_siginfo fdsi;
	struct sigaction newact;
	struct itimerspec its;
	struct sigevent sev;
	timer_t timerid;
	sigset_t mask;
	ssize_t s;
	int sfd;

	newact.sa_flags = SA_SIGINFO | SA_RESETHAND;
	newact.sa_sigaction = handler;
	sigemptyset(&newact.sa_mask);

	if (sigaction(SIGRTMIN, &newact, NULL) == -1)
		bail("sigaction");

	memset(&sev, 0, sizeof(sev));
	sev.sigev_notify = SIGEV_SIGNAL;
	sev.sigev_signo = SIGRTMIN;
	sev.sigev_value = my_sigval;

	if (timer_create(CLOCK_REALTIME, &sev, &timerid) == -1)
		bail("timer_create");

	/* Timer expires just once */
	its.it_value.tv_sec = 0;
	its.it_value.tv_nsec = 100;
	its.it_interval.tv_sec = 0;
	its.it_interval.tv_nsec = 0;

	/* Arm timer */
	if (timer_settime(timerid, 0, &its, NULL) == -1)
                bail("timer_settime");

	/* Wait for timer to expire, which executes the handler */
	while (!handler_run)
		usleep(100000);

	/* Inhibit default SIGRTMIN handling */
	sigemptyset(&mask);
	sigaddset(&mask, SIGRTMIN);
	if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
		bail("sigprocmask");

	sfd = signalfd(-1, &mask, 0);
	if (sfd == -1)
		bail("signalfd");

	/* Re-arm timer */
	if (timer_settime(timerid, 0, &its, NULL) == -1)
                bail("timer_settime");

	/* Block until timer expiration */
	s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
	if (s != sizeof(struct signalfd_siginfo))
		bail("read");

	assert(fdsi.ssi_code == SI_TIMER);
	assert(fdsi.ssi_ptr ==
	       (unsigned long)my_sigval.sival_ptr); /* Succeeds */
	assert(fdsi.ssi_int == my_sigval.sival_int); /* Fails */

	return EXIT_SUCCESS;
}