On Mon, Nov 02, 2020 at 08:10:57PM -0800, Peter Collingbourne wrote:

On Mon, Nov 2, 2020 at 9:38 AM Dave Martin [off-list ref] wrote:

quoted

On Fri, Oct 16, 2020 at 05:12:32PM -0700, Peter Collingbourne wrote:

quoted

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.

[...]

quoted

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel