Re: [PATCH v12 7/8] signal: define the field siginfo.si_xflags

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Nov 02, 2020 at 08:10:57PM -0800, Peter Collingbourne wrote:
> On Mon, Nov 2, 2020 at 9:38 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Fri, Oct 16, 2020 at 05:12:32PM -0700, Peter Collingbourne wrote:
> > > This field will contain flags that may be used by signal handlers to
> > > determine whether other fields in the _sigfault portion of siginfo are
> > > valid. An example use case is the following patch, which introduces
> > > the si_addr_tag_bits{,_mask} fields.
> > >
> > > A new sigcontext flag, SA_XFLAGS, is introduced in order to allow
> > > a signal handler to require the kernel to set the field (but note
> > > that the field will be set anyway if the kernel supports the flag,
> > > regardless of its value). In combination with the previous patches,
> > > this allows a userspace program to determine whether the kernel will
> > > set the field.
> >
> > Apologies for this coming rather late:
> >
> > It occurs to me that we might want a more specific name, since this only
> > applies to fault signals -- say, SA_FAULTFLAGS.
> >
> > If we end up wanting to add flags fields for other signal types, then we
> > might end up needing a SA_ flag for each, which would be a bit annoying.
> >
> > So, alternatively. I wonder whether it's worth preemptively adding an
> > extra flags to every kind of kernel-generated siginfo.  If so, then
> > having a single SA_XFLAGS would be fine.
> >
> >
> > If added flags fields all over the place is considered overkill, then I
> > guess it's sufficient to rename this flag.
> >
> > If renaming, the actual flags field in siginfo should also be renamed to
> > match.
> 
> I'd prefer not to add flags fields to every union member at this
> point. I agree that faultflags is a better name, and I guess it's one
> more reason not to try and reuse the ia64 field. Renamed in v13.

Ack -- I thought I should make the point, but we've got enough spare
sa_flags bits for now to make this one SIL_FAULT-specific, providing the
SA_foo name looks equally specific -- so just renaming that should be OK.

If we end up adding a flags field to another siginfo union member in the
future, it's probably worth adding all the rest at the same time ... but
it may never happen.

[...]

> > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > index 43d6179508d6..85b5b4e38661 100644
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -687,18 +687,32 @@ static int ptrace_getsiginfo(struct task_struct *child, kernel_siginfo_t *info)
> > >       return error;
> > >  }
> > >
> > > -static int ptrace_setsiginfo(struct task_struct *child, const kernel_siginfo_t *info)
> > > +static int ptrace_setsiginfo(struct task_struct *child, unsigned long flags,
> > > +                          kernel_siginfo_t *info)
> > >  {
> > > -     unsigned long flags;
> > > +     unsigned long lock_flags;
> > >       int error = -ESRCH;
> > >
> > > -     if (lock_task_sighand(child, &flags)) {
> > > +     if (flags & ~PTRACE_SIGINFO_XFLAGS) {
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     /*
> > > +      * If the caller is unaware of si_xflags and we're using a layout that
> > > +      * requires it, set it to 0 which means "no fields are available".
> > > +      */
> > > +     if (!(flags & PTRACE_SIGINFO_XFLAGS) &&
> > > +         siginfo_layout_is_fault(
> > > +                 siginfo_layout(info->si_signo, info->si_code)))
> > > +             info->si_xflags = 0;
> > > +
> > > +     if (lock_task_sighand(child, &lock_flags)) {
> > >               error = -EINVAL;
> > >               if (likely(child->last_siginfo != NULL)) {
> > >                       copy_siginfo(child->last_siginfo, info);
> > >                       error = 0;
> > >               }
> > > -             unlock_task_sighand(child, &flags);
> > > +             unlock_task_sighand(child, &lock_flags);
> > >       }
> > >       return error;
> > >  }
> > > @@ -1038,9 +1052,12 @@ int ptrace_request(struct task_struct *child, long request,
> > >               break;
> > >
> > >       case PTRACE_SETSIGINFO:
> > > +             addr = 0;
> >
> > If this is intended to fall through, please add a
> >
> >                 /* fall through */
> >
> > comment here (newer GCC has warnings to catch this; not sure about
> > clang, but I'd be surprised if no version warns).
> 
> Yes, clang has this warning, but it looks like it is currently
> disabled in clang due to differences between the compilers [1] so I
> didn't see it.
> 
> It looks like the kernel is moving towards using the fallthrough
> macro/attribute defined in include/linux/compiler_attributes.h (and to
> me this personally seems better than relying on parsing comments), so
> I've used that macro in v13.

Ah, I wasn't aware of that.  Sounds better!

Cheers
---Dave