linux-user/syscall.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
From: Shu-Chun Weng <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
Reviewed-by: Patrick Venture <venture@google.com>
Signed-off-by: Shu-Chun Weng <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;
--
2.34.1.448.ga2b2bfdf31-goog
Le 06/01/2022 à 23:00, Patrick Venture a écrit :
> From: Shu-Chun Weng <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
> Reviewed-by: Patrick Venture <venture@google.com>
> Signed-off-by: Shu-Chun Weng <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
successfully.
Could you also update TARGET_NR_sigprocmask in the same way as it seems the kernel behaves like this
too in this case?
Thanks,
Laurent
On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 06/01/2022 à 23:00, Patrick Venture a écrit :
> > From: Shu-Chun Weng <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
> > Reviewed-by: Patrick Venture <venture@google.com>
> > Signed-off-by: Shu-Chun Weng <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
> 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.
>
> Thanks,
> Laurent
>
Patrick
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.
Thanks,
Laurent
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.
>
> Thanks,
> Laurent
>
>
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
>>
>>
© 2016 - 2024 Red Hat, Inc.