signalfd: fill in ssi_int for posix timers and message queues
PATCH 1/1 signalfd: fill in ssi_int for posix timers and message queues
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.
--- fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
fs/signalfd.c +2 -0
@@ -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 :/*
Signature · 1 line
1.6.6.1
Quoted 31 lines · Nathan Lynch
If signalfd is used to consume a signal generated by a POSIX interval timer or POSIX message queue, the
ssi_intfield does not reflect the data (sigevent->sigev_value) supplied totimer_create(2)ormq_notify(3). (Thessi_ptrfield, 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_intin the remaining cases (__SI_TIMER,__SI_MESGQ) wheressi_ptris set.Signed-off-by Nathan Lynch <[email protected]>--- fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)fs/signalfd.c +1
-0@@ -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);
fs/signalfd.c static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, · line 117
Quoted 6 lines
fs/signalfd.c +1
-0@@ -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?
If you do not want to, let me know and I'll do it.
- Davide
Hello Davide,
Quoted 37 lines · Davide Libenzi
Quoted 35 lines · Nathan Lynch
If signalfd is used to consume a signal generated by a POSIX interval timer or POSIX message queue, the
ssi_intfield does not reflect the data (sigevent->sigev_value) supplied totimer_create(2)ormq_notify(3). (Thessi_ptrfield, 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_intin the remaining cases (__SI_TIMER,__SI_MESGQ) wheressi_ptris set.Signed-off-by Nathan Lynch <[email protected]>--- fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)fs/signalfd.c +2
-0@@ -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);
Quoted 8 lines
fs/signalfd.c static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, · line 117
Quoted 2 lines
fs/signalfd.c +0
-0break ;default :I am fine with it, but I now noticed that
signalfd_copyinfo()got out of sync fromcopy_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 incopy_siginfo_to_user()tosignalfd_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.
#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);
}#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;
}Quoted 57 lines · Nathan Lynch
Hello Davide,
Quoted 37 lines · Davide Libenzi
Quoted 35 lines · Nathan Lynch
If signalfd is used to consume a signal generated by a POSIX interval timer or POSIX message queue, the
ssi_intfield does not reflect the data (sigevent->sigev_value) supplied totimer_create(2)ormq_notify(3). (Thessi_ptrfield, 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_intin the remaining cases (__SI_TIMER,__SI_MESGQ) wheressi_ptris set.Signed-off-by Nathan Lynch <[email protected]>--- fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)fs/signalfd.c +2
-0@@ -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);Quoted 8 lines
fs/signalfd.c static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, · line 117
Quoted 2 lines
fs/signalfd.c +0
-0break ;default :I am fine with it, but I now noticed that
signalfd_copyinfo()got out of sync fromcopy_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 incopy_siginfo_to_user()tosignalfd_copyinfo()too?Sorry, I'm not sure I understand. Are you saying that
copy_siginfo_to_usershould have analogous lines added to assign tosi_int? That's actually not necessary if I read the code correctly: in struct siginfo,si_ptrandsi_intare members of a sigval union, so assigning to the former covers the latter. signalfd must assign bothssi_ptrandssi_intsince they occupy different locations insignalfd_siginfo.
Quoted 2 lines
Perhaps the attached testcases make the problem (as I see it) more clear? The final assertion fails without this patch.
Sorry, my bad. I had forgotten that siginfo had them in a union, so the different code in signalfd_copyinfo() is needed.
Patch looks fine to me as is.
- Davide
Quoted 5 lines · On Fri, 02 Jul 2010 19:38:48 -0500 Nathan Lynch <[email protected]> wrote:
If signalfd is used to consume a signal generated by a POSIX interval timer or POSIX message queue, the
ssi_intfield does not reflect the data (sigevent->sigev_value) supplied totimer_create(2)ormq_notify(3). (Thessi_ptrfield, however, is filled in.)
Quoted 8 lines
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_intin the remaining cases (__SI_TIMER,__SI_MESGQ) wheressi_ptris set.
This introduces an incompatibility between kernel versions. Someone develops and tests an application on 2.6.36 or later then ships it and lo, it malfunctions on 2.6.35 and earlier.
Is there a way to avoid that? Don't think so.
How should the more-awake-than-average application developer prevent this problem? Should he probe the syscall at runtime to determine its behaviour? He can't use the kernel version number because the kernel provider might have backported this patch into an earlier kernel.
We can minimise the problem by backporting into -stable, and hoping that awake kernel packagers understand the issue, and backport the change as far as they can.
So it's not 100% obvious that this change is desirable. Does the functionality which this patch adds justify the introduction of these problems?
Quoted 9 lines
--- fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)fs/signalfd.c +0
-0
fs/signalfd.c static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, · line 92
Quoted 5 lines
fs/signalfd.c +1
-0@@ -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 ;
hm, someone bollixed the __SI_TIMER indenting.
Quoted 23 lines · Andrew Morton
Quoted 5 lines · On Fri, 02 Jul 2010 19:38:48 -0500 Nathan Lynch <[email protected]> wrote:
If signalfd is used to consume a signal generated by a POSIX interval timer or POSIX message queue, the
ssi_intfield does not reflect the data (sigevent->sigev_value) supplied totimer_create(2)ormq_notify(3). (Thessi_ptrfield, however, is filled in.)Quoted 8 lines
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_intin the remaining cases (__SI_TIMER,__SI_MESGQ) wheressi_ptris set.This introduces an incompatibility between kernel versions. Someone develops and tests an application on 2.6.36 or later then ships it and lo, it malfunctions on 2.6.35 and earlier.
Is there a way to avoid that? Don't think so.
Quoted 8 lines
How should the more-awake-than-average application developer prevent this problem? Should he probe the syscall at runtime to determine its behaviour? He can't use the kernel version number because the kernel provider might have backported this patch into an earlier kernel.
We can minimise the problem by backporting into -stable, and hoping that awake kernel packagers understand the issue, and backport the change as far as they can.
And perhaps document it in the signalfd man page. This was done for commit 0859ab5 "signalfd: fix for incorrect SI_QUEUE user data reporting", which seems to a be a similar case.
Quoted 3 lines
So it's not 100% obvious that this change is desirable. Does the functionality which this patch adds justify the introduction of these problems?
I think the change is desirable in that no user of the interface could reasonably expect the current behavior with respect to the ssi_int field, and that it reconciles signalfd's behavior with its design intentions. On the other hand, I noticed this discrepancy only because I was cribbing signalfd's data structures for checkpoint/restart, not because I am aware of any application that is affected, nor was I able to find one using Google's code search. It would be highly speculative of me to say that no application depends on the current behavior, but it is difficult to imagine a correctly functioning application that depends on it.
Davide, any opinion here?
Quoted 15 lines
fs/signalfd.c static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, · line 92
Quoted 13 lines
fs/signalfd.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)fs/signalfd.c +1
-0@@ -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 ;hm, someone bollixed the
__SI_TIMERindenting.
In kernel/signal.c::copy_siginfo_to_user() too :)
Quoted 4 lines · On Tue, 20 Jul 2010 22:44:19 -0500 Nathan Lynch <[email protected]> wrote:
Quoted 3 lines
So it's not 100% obvious that this change is desirable. Does the functionality which this patch adds justify the introduction of these problems?
Quoted 10 lines
I think the change is desirable in that no user of the interface could reasonably expect the current behavior with respect to the
ssi_intfield, and that it reconciles signalfd's behavior with its design intentions. On the other hand, I noticed this discrepancy only because I was cribbing signalfd's data structures for checkpoint/restart, not because I am aware of any application that is affected, nor was I able to find one using Google's code search. It would be highly speculative of me to say that no application depends on the current behavior, but it is difficult to imagine a correctly functioning application that depends on it.
It's not a matter of a current application depending on current behaviour! The problem is that an application written in 2018 which depends on the _new_ behaviour will not work on 2.6.34.
It wouldn't be the worst thing we've ever done to our long-suffering users, but it is a permanent cost of having screwed things up :(
Quoted 17 lines · Andrew Morton
Quoted 4 lines · On Tue, 20 Jul 2010 22:44:19 -0500 Nathan Lynch <[email protected]> wrote:
Quoted 3 lines
So it's not 100% obvious that this change is desirable. Does the functionality which this patch adds justify the introduction of these problems?
Quoted 10 lines
I think the change is desirable in that no user of the interface could reasonably expect the current behavior with respect to the
ssi_intfield, and that it reconciles signalfd's behavior with its design intentions. On the other hand, I noticed this discrepancy only because I was cribbing signalfd's data structures for checkpoint/restart, not because I am aware of any application that is affected, nor was I able to find one using Google's code search. It would be highly speculative of me to say that no application depends on the current behavior, but it is difficult to imagine a correctly functioning application that depends on it.
Quoted 3 lines
It's not a matter of a current application depending on current behaviour! The problem is that an application written in 2018 which depends on the _new_ behaviour will not work on 2.6.34.
Yes, I misinterpreted your concern, sorry. But I've never understood Linux to make promises with respect to forward compatibility at the system call layer. Bug fixes[1] and features[2] that, like this patch, break that compatibility seem to have gone in without raising this issue.
Am I mistaken? Or has there been a change in policy I've missed?
[1] "signalfd: fix for incorrect SI_QUEUE user data reporting" (0859ab5)
[2] "hugetlb: add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions" (4e52780)
Quoted 6 lines · Andrew Morton
It's not a matter of a current application depending on current behaviour! The problem is that an application written in 2018 which depends on the _new_ behaviour will not work on 2.6.34.
It wouldn't be the worst thing we've ever done to our long-suffering users, but it is a permanent cost of having screwed things up :(
While I agree it is a problem with the kernel compatibility handling, every time we add new features to an existing interface we end up in similar scenarios.
Nowadays a "require kernel >= X.Y.Z" is all but an uncommon case for userspace code.
- Davide