On Tue, Jan 11, 2022 at 3:06 PM Patrick Venture <venture@google.com> wrote:

>
>
> On Tue, Jan 11, 2022 at 12:50 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
>> Hi Patrick,
>>
>> Le 11/01/2022 à 21:14, Patrick Venture a écrit :
>> >
>> >
>> > On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu
>> <mailto:laurent@vivier.eu>> wrote:
>> >
>> >     Le 06/01/2022 à 23:00, Patrick Venture a écrit :
>> >      > From: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>>
>> >      >
>> >      > Linux kernel does it this way (checks read permission before
>> validating `how`)
>> >      > and the latest version of ABSL's `AddressIsReadable()` depends
>> on this
>> >      > behavior.
>> >      >
>> >      > c.f.
>> >
>> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
>> >     <
>> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
>> >
>> >      > Reviewed-by: Patrick Venture <venture@google.com <mailto:
>> venture@google.com>>
>> >      > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto:
>> scw@google.com>>
>> >      > ---
>> >      >   linux-user/syscall.c | 10 +++++-----
>> >      >   1 file changed, 5 insertions(+), 5 deletions(-)
>> >      >
>> >      > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> >      > index ce9d64896c..3070d31f34 100644
>> >      > --- a/linux-user/syscall.c
>> >      > +++ b/linux-user/syscall.c
>> >      > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void
>> *cpu_env, int num, abi_long arg1,
>> >      >               }
>> >      >
>> >      >               if (arg2) {
>> >      > +                if (!(p = lock_user(VERIFY_READ, arg2,
>> sizeof(target_sigset_t), 1)))
>> >      > +                    return -TARGET_EFAULT;
>> >      > +                target_to_host_sigset(&set, p);
>> >      > +                unlock_user(p, arg2, 0);
>> >      > +                set_ptr = &set;
>> >      >                   switch(how) {
>> >      >                   case TARGET_SIG_BLOCK:
>> >      >                       how = SIG_BLOCK;
>> >      > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void
>> *cpu_env, int num, abi_long arg1,
>> >      >                   default:
>> >      >                       return -TARGET_EINVAL;
>> >      >                   }
>> >      > -                if (!(p = lock_user(VERIFY_READ, arg2,
>> sizeof(target_sigset_t), 1)))
>> >      > -                    return -TARGET_EFAULT;
>> >      > -                target_to_host_sigset(&set, p);
>> >      > -                unlock_user(p, arg2, 0);
>> >      > -                set_ptr = &set;
>> >      >               } else {
>> >      >                   how = 0;
>> >      >                   set_ptr = NULL;
>> >
>> >     I know it's only code move but generally we also update the style
>> to pass scripts/checkpatch.pl
>> >     <http://checkpatch.pl>
>> >     successfully.
>> >
>> >
>> > That is a reasonable request, however, can I just send a follow-on
>> patch?  I didn't write this one
>> > and I honestly don't know much about it, but I don't mind doing the
>> cleanup
>> >
>> >
>> >     Could you also update TARGET_NR_sigprocmask in the same way as it
>> seems the kernel behaves like
>> >     this
>> >     too in this case?
>> >
>> >
>> > I can take a look.  I would prefer then to also prefetch the style
>> fixup in a preceding patch. I
>> > don't recall seeing whether qemu supports clang-format.
>> >
>>
>> There is no problem. You can keep this patch unmodified, and add patches
>> to fix the problems.
>>
>> I only ask to have all the patches in one series.
>>
>
> Will take a swing at this for v2.
>

Laurent,
I spent some time today going over the patches and digging into what
they're actually doing and I think I can make this two patches, both with
the style changes squashed and will send in a v2 today.

Thanks


>
>
>>
>> Thanks,
>> Laurent
>>
>>