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 <[email protected]>
---
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:
/*
--
1.6.6.1
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 <[email protected]>
> ---
> 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?
If you do not want to, let me know and I'll do it.
- Davide
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 <[email protected]>
> > ---
> > 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.
On Mon, 5 Jul 2010, Nathan Lynch wrote:
> 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 <[email protected]>
> > > ---
> > > 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.
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
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_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.
>
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?
> ---
> 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;
hm, someone bollixed the __SI_TIMER indenting.
On Tue, 2010-07-20 at 15:42 -0700, Andrew Morton wrote:
> 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_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.
> >
>
> 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.
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.
> 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?
> > 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;
>
> hm, someone bollixed the __SI_TIMER indenting.
In kernel/signal.c::copy_siginfo_to_user() too :)
On Tue, 20 Jul 2010 22:44:19 -0500 Nathan Lynch <[email protected]> wrote:
> > 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.
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 :(
On Tue, 2010-07-20 at 21:10 -0700, Andrew Morton wrote:
> On Tue, 20 Jul 2010 22:44:19 -0500 Nathan Lynch <[email protected]> wrote:
>
> > > 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.
>
> 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)
On Tue, 20 Jul 2010, Andrew Morton wrote:
> 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