* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-29 20:14 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Florian Weimer, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181129195551.woe2bl3z3yaysqb6@brauner.io>
> On Nov 29, 2018, at 11:55 AM, Christian Brauner <christian@brauner.io> wrote:
>
>> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
>>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
>>>
>>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>>
>>>>> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>> Disclaimer: I'm looking at this patch because Christian requested it.
>>>>> I'm not a kernel developer.
>>>>>
>>>>> * Christian Brauner:
>>>>>
>>>>>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
>>>> b/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> index 3cf7b533b3d1..3f27ffd8ae87 100644
>>>>>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
>>>>>> @@ -398,3 +398,4 @@
>>>>>> 384 i386 arch_prctl sys_arch_prctl
>>>> __ia32_compat_sys_arch_prctl
>>>>>> 385 i386 io_pgetevents sys_io_pgetevents
>>>> __ia32_compat_sys_io_pgetevents
>>>>>> 386 i386 rseq sys_rseq __ia32_sys_rseq
>>>>>> +387 i386 procfd_signal sys_procfd_signal
>>>> __ia32_compat_sys_procfd_signal
>>>>>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
>>>> b/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> index f0b1709a5ffb..8a30cde82450 100644
>>>>>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
>>>>>> @@ -343,6 +343,7 @@
>>>>>> 332 common statx __x64_sys_statx
>>>>>> 333 common io_pgetevents __x64_sys_io_pgetevents
>>>>>> 334 common rseq __x64_sys_rseq
>>>>>> +335 64 procfd_signal __x64_sys_procfd_signal
>>>>>>
>>>>>> #
>>>>>> # x32-specific system call numbers start at 512 to avoid cache
>>>> impact
>>>>>> @@ -386,3 +387,4 @@
>>>>>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
>>>>>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
>>>>>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
>>>>>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
>>>>>
>>>>> Is there a reason why these numbers have to be different?
>>>>>
>>>>> (See the recent discussion with Andy Lutomirski.)
>>>>
>>>> Hah, I missed this part of the patch. Let’s not add new x32 syscall
>>>> numbers.
>>>>
>>>> Also, can we perhaps rework this a bit to get rid of the compat entry
>>>> point? The easier way would be to check in_compat_syscall(). The nicer
>>>> way IMO would be to use the 64-bit structure for 32-bit as well.
>>>
>>> Do you have a syscall which set precedence/did this before I could look at?
>>> Just if you happen to remember one.
>>> Fwiw, I followed the other signal syscalls.
>>> They all introduce compat syscalls.
>>>
>>
>> Not really.
>>
>> Let me try to explain. I have three issues with the approach in your patchset:
>>
>> 1. You're introducing a new syscall, and it behaves differently on
>> 32-bit and 64-bit because the structure you pass in is different.
>> This is necessary for old syscalls where compatibility matters, but
>> maybe we can get rid of it for new syscalls. Could we define a
>> siginfo64_t that is identical to the 64-bit siginfo_t and just use
>> that in all cases?
>>
>> 2. Assuming that #1 doesn't work, then we need compat support. But
>> you're doing it by having two different entry points. Instead, you
>> could have a single entry point that calls in_compat_syscall() to
>> decide which structure to read. This would simplify things because
>> x86 doesn't really support the separate compat entry points, which
>> leads me to #3.
>>
>> 3. The separate x32 numbers are a huge turd that may have security
>> holes and certainly have comprehensibility holes. I will object to
>> any patch that adds a new one (like yours). Fixing #1 or #2 makes
>> this problem go away.
>>
>> Does that make any sense? The #2 fix would be something like:
>>
>> if (in_compat_syscall)
>> copy...user32();
>> else
>> copy_from_user();
>>
>> The #1 fix would add a copy_siginfo_from_user64() or similar.
>
> Thanks very much! That all helped a bunch already! I'll try to go the
> copy_siginfo_from_user64() way first and see if I can make this work. If
> we do this I would however only want to use it for the new syscall first
> and not change all other signal syscalls over to it too. I'd rather keep
> this patchset focussed and small and do such conversions caused by the
> new approach later. Does that sound reasonable?
Absolutely. I don’t think we can change old syscalls — the ABI is set in stone. But for new syscalls, I think the always-64-bit behavior makes sense.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-29 19:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Daniel Colascione, Tim Murray,
linux-man, Kees Cook
In-Reply-To: <CALCETrWnQNMQcCmFZrftVVYgAMW6DT3gyxvVb_v9_enUCUkHiw@mail.gmail.com>
On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:
> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
> >
> > On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
> > >
> > >
> > >> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
> > >wrote:
> > >>
> > >> Disclaimer: I'm looking at this patch because Christian requested it.
> > >> I'm not a kernel developer.
> > >>
> > >> * Christian Brauner:
> > >>
> > >>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
> > >b/arch/x86/entry/syscalls/syscall_32.tbl
> > >>> index 3cf7b533b3d1..3f27ffd8ae87 100644
> > >>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > >>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > >>> @@ -398,3 +398,4 @@
> > >>> 384 i386 arch_prctl sys_arch_prctl
> > >__ia32_compat_sys_arch_prctl
> > >>> 385 i386 io_pgetevents sys_io_pgetevents
> > >__ia32_compat_sys_io_pgetevents
> > >>> 386 i386 rseq sys_rseq __ia32_sys_rseq
> > >>> +387 i386 procfd_signal sys_procfd_signal
> > >__ia32_compat_sys_procfd_signal
> > >>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
> > >b/arch/x86/entry/syscalls/syscall_64.tbl
> > >>> index f0b1709a5ffb..8a30cde82450 100644
> > >>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > >>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > >>> @@ -343,6 +343,7 @@
> > >>> 332 common statx __x64_sys_statx
> > >>> 333 common io_pgetevents __x64_sys_io_pgetevents
> > >>> 334 common rseq __x64_sys_rseq
> > >>> +335 64 procfd_signal __x64_sys_procfd_signal
> > >>>
> > >>> #
> > >>> # x32-specific system call numbers start at 512 to avoid cache
> > >impact
> > >>> @@ -386,3 +387,4 @@
> > >>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
> > >>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
> > >>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
> > >>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
> > >>
> > >> Is there a reason why these numbers have to be different?
> > >>
> > >> (See the recent discussion with Andy Lutomirski.)
> > >
> > >Hah, I missed this part of the patch. Let’s not add new x32 syscall
> > >numbers.
> > >
> > >Also, can we perhaps rework this a bit to get rid of the compat entry
> > >point? The easier way would be to check in_compat_syscall(). The nicer
> > >way IMO would be to use the 64-bit structure for 32-bit as well.
> >
> > Do you have a syscall which set precedence/did this before I could look at?
> > Just if you happen to remember one.
> > Fwiw, I followed the other signal syscalls.
> > They all introduce compat syscalls.
> >
>
> Not really.
>
> Let me try to explain. I have three issues with the approach in your patchset:
>
> 1. You're introducing a new syscall, and it behaves differently on
> 32-bit and 64-bit because the structure you pass in is different.
> This is necessary for old syscalls where compatibility matters, but
> maybe we can get rid of it for new syscalls. Could we define a
> siginfo64_t that is identical to the 64-bit siginfo_t and just use
> that in all cases?
>
> 2. Assuming that #1 doesn't work, then we need compat support. But
> you're doing it by having two different entry points. Instead, you
> could have a single entry point that calls in_compat_syscall() to
> decide which structure to read. This would simplify things because
> x86 doesn't really support the separate compat entry points, which
> leads me to #3.
>
> 3. The separate x32 numbers are a huge turd that may have security
> holes and certainly have comprehensibility holes. I will object to
> any patch that adds a new one (like yours). Fixing #1 or #2 makes
> this problem go away.
>
> Does that make any sense? The #2 fix would be something like:
>
> if (in_compat_syscall)
> copy...user32();
> else
> copy_from_user();
>
> The #1 fix would add a copy_siginfo_from_user64() or similar.
Thanks very much! That all helped a bunch already! I'll try to go the
copy_siginfo_from_user64() way first and see if I can make this work. If
we do this I would however only want to use it for the new syscall first
and not change all other signal syscalls over to it too. I'd rather keep
this patchset focussed and small and do such conversions caused by the
new approach later. Does that sound reasonable?
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-29 19:22 UTC (permalink / raw)
To: Christian Brauner
Cc: Florian Weimer, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Lutomirski, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Daniel Colascione, Tim Murray, linux-man, Kees Cook
In-Reply-To: <993B98AC-51DF-4131-AF7F-7DA2A7F485F1@brauner.io>
On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner <christian@brauner.io> wrote:
>
> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >
> >> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
> >wrote:
> >>
> >> Disclaimer: I'm looking at this patch because Christian requested it.
> >> I'm not a kernel developer.
> >>
> >> * Christian Brauner:
> >>
> >>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
> >b/arch/x86/entry/syscalls/syscall_32.tbl
> >>> index 3cf7b533b3d1..3f27ffd8ae87 100644
> >>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> >>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> >>> @@ -398,3 +398,4 @@
> >>> 384 i386 arch_prctl sys_arch_prctl
> >__ia32_compat_sys_arch_prctl
> >>> 385 i386 io_pgetevents sys_io_pgetevents
> >__ia32_compat_sys_io_pgetevents
> >>> 386 i386 rseq sys_rseq __ia32_sys_rseq
> >>> +387 i386 procfd_signal sys_procfd_signal
> >__ia32_compat_sys_procfd_signal
> >>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
> >b/arch/x86/entry/syscalls/syscall_64.tbl
> >>> index f0b1709a5ffb..8a30cde82450 100644
> >>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> >>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> >>> @@ -343,6 +343,7 @@
> >>> 332 common statx __x64_sys_statx
> >>> 333 common io_pgetevents __x64_sys_io_pgetevents
> >>> 334 common rseq __x64_sys_rseq
> >>> +335 64 procfd_signal __x64_sys_procfd_signal
> >>>
> >>> #
> >>> # x32-specific system call numbers start at 512 to avoid cache
> >impact
> >>> @@ -386,3 +387,4 @@
> >>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
> >>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
> >>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
> >>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
> >>
> >> Is there a reason why these numbers have to be different?
> >>
> >> (See the recent discussion with Andy Lutomirski.)
> >
> >Hah, I missed this part of the patch. Let’s not add new x32 syscall
> >numbers.
> >
> >Also, can we perhaps rework this a bit to get rid of the compat entry
> >point? The easier way would be to check in_compat_syscall(). The nicer
> >way IMO would be to use the 64-bit structure for 32-bit as well.
>
> Do you have a syscall which set precedence/did this before I could look at?
> Just if you happen to remember one.
> Fwiw, I followed the other signal syscalls.
> They all introduce compat syscalls.
>
Not really.
Let me try to explain. I have three issues with the approach in your patchset:
1. You're introducing a new syscall, and it behaves differently on
32-bit and 64-bit because the structure you pass in is different.
This is necessary for old syscalls where compatibility matters, but
maybe we can get rid of it for new syscalls. Could we define a
siginfo64_t that is identical to the 64-bit siginfo_t and just use
that in all cases?
2. Assuming that #1 doesn't work, then we need compat support. But
you're doing it by having two different entry points. Instead, you
could have a single entry point that calls in_compat_syscall() to
decide which structure to read. This would simplify things because
x86 doesn't really support the separate compat entry points, which
leads me to #3.
3. The separate x32 numbers are a huge turd that may have security
holes and certainly have comprehensibility holes. I will object to
any patch that adds a new one (like yours). Fixing #1 or #2 makes
this problem go away.
Does that make any sense? The #2 fix would be something like:
if (in_compat_syscall)
copy...user32();
else
copy_from_user();
The #1 fix would add a copy_siginfo_from_user64() or similar.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-29 19:16 UTC (permalink / raw)
To: Andy Lutomirski, Florian Weimer
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net>
On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com>
>wrote:
>>
>> Disclaimer: I'm looking at this patch because Christian requested it.
>> I'm not a kernel developer.
>>
>> * Christian Brauner:
>>
>>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
>b/arch/x86/entry/syscalls/syscall_32.tbl
>>> index 3cf7b533b3d1..3f27ffd8ae87 100644
>>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
>>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
>>> @@ -398,3 +398,4 @@
>>> 384 i386 arch_prctl sys_arch_prctl
>__ia32_compat_sys_arch_prctl
>>> 385 i386 io_pgetevents sys_io_pgetevents
>__ia32_compat_sys_io_pgetevents
>>> 386 i386 rseq sys_rseq __ia32_sys_rseq
>>> +387 i386 procfd_signal sys_procfd_signal
>__ia32_compat_sys_procfd_signal
>>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl
>b/arch/x86/entry/syscalls/syscall_64.tbl
>>> index f0b1709a5ffb..8a30cde82450 100644
>>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
>>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
>>> @@ -343,6 +343,7 @@
>>> 332 common statx __x64_sys_statx
>>> 333 common io_pgetevents __x64_sys_io_pgetevents
>>> 334 common rseq __x64_sys_rseq
>>> +335 64 procfd_signal __x64_sys_procfd_signal
>>>
>>> #
>>> # x32-specific system call numbers start at 512 to avoid cache
>impact
>>> @@ -386,3 +387,4 @@
>>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
>>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
>>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
>>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
>>
>> Is there a reason why these numbers have to be different?
>>
>> (See the recent discussion with Andy Lutomirski.)
>
>Hah, I missed this part of the patch. Let’s not add new x32 syscall
>numbers.
>
>Also, can we perhaps rework this a bit to get rid of the compat entry
>point? The easier way would be to check in_compat_syscall(). The nicer
>way IMO would be to use the 64-bit structure for 32-bit as well.
Do you have a syscall which set precedence/did this before I could look at?
Just if you happen to remember one.
Fwiw, I followed the other signal syscalls.
They all introduce compat syscalls.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-29 16:54 UTC (permalink / raw)
To: Florian Weimer
Cc: Christian Brauner, ebiederm, linux-kernel, serge, jannh, luto,
akpm, oleg, cyphar, viro, linux-fsdevel, linux-api, dancol,
timmurray, linux-man, Kees Cook
In-Reply-To: <87in0g5aqo.fsf@oldenburg.str.redhat.com>
> On Nov 29, 2018, at 4:28 AM, Florian Weimer <fweimer@redhat.com> wrote:
>
> Disclaimer: I'm looking at this patch because Christian requested it.
> I'm not a kernel developer.
>
> * Christian Brauner:
>
>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
>> index 3cf7b533b3d1..3f27ffd8ae87 100644
>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
>> @@ -398,3 +398,4 @@
>> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
>> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
>> 386 i386 rseq sys_rseq __ia32_sys_rseq
>> +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal
>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
>> index f0b1709a5ffb..8a30cde82450 100644
>> --- a/arch/x86/entry/syscalls/syscall_64.tbl
>> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
>> @@ -343,6 +343,7 @@
>> 332 common statx __x64_sys_statx
>> 333 common io_pgetevents __x64_sys_io_pgetevents
>> 334 common rseq __x64_sys_rseq
>> +335 64 procfd_signal __x64_sys_procfd_signal
>>
>> #
>> # x32-specific system call numbers start at 512 to avoid cache impact
>> @@ -386,3 +387,4 @@
>> 545 x32 execveat __x32_compat_sys_execveat/ptregs
>> 546 x32 preadv2 __x32_compat_sys_preadv64v2
>> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
>> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
>
> Is there a reason why these numbers have to be different?
>
> (See the recent discussion with Andy Lutomirski.)
Hah, I missed this part of the patch. Let’s not add new x32 syscall numbers.
Also, can we perhaps rework this a bit to get rid of the compat entry point? The easier way would be to check in_compat_syscall(). The nicer way IMO would be to use the 64-bit structure for 32-bit as well.
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Jürg Billeter @ 2018-11-29 15:41 UTC (permalink / raw)
To: Oleg Nesterov, Eric W. Biederman
Cc: Andrew Morton, Thomas Gleixner, Kees Cook, Andy Lutomirski,
linux-api, linux-kernel
In-Reply-To: <20181129123409.GA10645@redhat.com>
Hi Oleg,
Thanks for the review.
On Thu, 2018-11-29 at 13:34 +0100, Oleg Nesterov wrote:
> To me it would be more clean to call walk_process_tree(kill_descendant_visitor)
> unconditionally in find_new_reaper() right before "if (has_child_subreaper)", but
> then we will need to shift read_lock(tasklist) from walk_process_tree().
Yes, that's the reason why I added the call before the tasklist lock.
Let me know if you want me to move the read lock from
walk_process_tree() to PR_SET_CHILD_SUBREAPER (the only caller)
instead.
> So I think the patch is mostly fine, the only problem I can see is that
> PR_SET_KILL_DESCENDANTS_ON_EXIT can race with PR_SET_CHILD_SUBREAPER, they both
> need to update the bits in the same word.
Good point. I'll make it a regular bool instead of a bitfield for v2,
unless you have another approach in mind to fix this.
Jürg
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Oleg Nesterov @ 2018-11-29 15:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dmitry V. Levin, Kees Cook, Jann Horn, Michael Ellerman,
Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
Linux API, Ingo Molnar, strace-devel
In-Reply-To: <CALCETrVjMorQDsJzQGaREEB5_7B24ApWSoNQfRd3K3QNSGAyEA@mail.gmail.com>
On 11/28, Andy Lutomirski wrote:
>
> I don't like any of this at all. Can we please choose a sensible API
> design and let the API drive the implementation instead of vice versa?
I too do not understand your concerns...
> ISTM the correct solution is to add some new state to task_struct for
> this.
Sure we can do this. I have argued with the previous version not because
the new member blows the task_struct. Although I think it is better to avoid
this if possible.
But this doesn't affect the API.
Yes, this version uses ->ptrace_message but I think this is _good_ exactly
because it is already visible to userspace, so if debugger only needs to
distinguish syscall entry/exit it can simply use PTRACE_GETEVENTMSG without
PTRACE_GET_SYSCALL_INFO.
> If we're concerned about making task_struct bigger, I have a
> half-finished patch to factor all the ptrace tracee state into a
> separate struct.
I even sent the patch(es) which does this several years ago ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Oleg Nesterov @ 2018-11-29 14:47 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
Andy Lutomirski, linux-api, Ingo Molnar, strace-devel
In-Reply-To: <20181128221125.GA2800@altlinux.org>
On 11/29, Dmitry V. Levin wrote:
>
> 2. Document these values
sure, they should be documented and live in include/uapi/,
> chosen to avoid collisions with ptrace_message values
> set by other ptrace events
this is what I can't understand. But to clarify, I don't really care and
won't argue.
If an application wants to use PTRACE_GETEVENTMSG to distinguish entry/exit
(without PTRACE_GET_SYSCALL_INFO) it needs to do wait(status) and check status
anyway, otherwise PTRACE_GETEVENTMSG is simply pointless (wrt syscall entry/
exit). So we do not care if PTRACE_EVENTMSG_SYSCALL_ENTRY conflicts with, say,
SECCOMP_RET_DATA.
> so that PTRACE_GETEVENTMSG users can easily tell
> whether this new semantics is supported by the kernel or not.
Yes. And how much this can help? Again, an application can trivially detect
if this feature implemented or not, and it should do this anyway if it wants
to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
Again, I won't reallly argue. But if you insist that these values must be
unique then you probably need to add
BUILD_BUG_ON(PTRACE_EVENTMSG_SYSCALL_ENTRY <= PID_MAX_LIMIT);
?
OK, please forget...
Oleg.
^ permalink raw reply
* Re: [PATCH v6 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-11-29 13:05 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jann Horn, James Bottomley, kernel list, Linux API, containers,
dima, Al Viro, linux-fsdevel, Andrew Morton
In-Reply-To: <87zhusq3x7.fsf@xmission.com>
Le 01/11/2018 à 15:16, Eric W. Biederman a écrit :
> Laurent Vivier <laurent@vivier.eu> writes:
>
>> On 01/11/2018 04:51, Jann Horn wrote:
>>> On Thu, Nov 1, 2018 at 3:59 AM James Bottomley
>>> <James.Bottomley@hansenpartnership.com> wrote:
>>>>
>>>> On Tue, 2018-10-16 at 11:52 +0200, Laurent Vivier wrote:
>>>>> Hi,
>>>>>
>>>>> Any comment on this last version?
>>>>>
>>>>> Any chance to be merged?
>>>>
>>>> I've got a use case for this: I went to one of the Graphene talks in
>>>> Edinburgh and it struck me that we seem to keep reinventing the type of
>>>> sandboxing that qemu-user already does. However if you want to do an
>>>> x86 on x86 sandbox, you can't currently use the binfmt_misc mechanism
>>>> because that has you running *every* binary on the system emulated.
>>>> Doing it per user namespace fixes this problem and allows us to at
>>>> least cut down on all the pointless duplication.
>>>
>>> Waaaaaait. What? qemu-user does not do "sandboxing". qemu-user makes
>>> your code slower and *LESS* secure. As far as I know, qemu-user is
>>> only intended for purposes like development and testing.
>>>
>>
>> I think the idea here is not to run qemu, but to use an interpreter
>> (something like gVisor) into a container to control the binaries
>> execution inside the container without using this interpreter on the
>> host itself (container and host shares the same binfmt_misc
>> magic/mask).
>
> Please remind me of this patchset after the merge window is over, and if
> there are no issues I will take it via my user namespace branch.
>
> Last I looked I had a concern that some of the permission check issues
> were being papered over by using override cred instead of fixing the
> deaper code. Sometimes they are necessary but seeing work-arounds
> instead of fixes for problems tends to be a maintenance issue, possibly
> with security consequences. Best is if the everyone agrees on how all
> of the interfaces work so their are no surprises.
I don't know where we are in the merge window, but is there something I
can do to have this merged?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Oleg Nesterov @ 2018-11-29 12:34 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jürg Billeter, Andrew Morton, Thomas Gleixner, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <87r2f5gr9g.fsf@xmission.com>
On 11/28, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 11/27, Jürg Billeter wrote:
> >>
> >> @@ -704,6 +713,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> >> struct task_struct *p, *n;
> >> LIST_HEAD(dead);
> >>
> >> + if (group_dead && tsk->signal->kill_descendants_on_exit)
> >> + walk_process_tree(tsk, kill_descendant_visitor, NULL);
> >
> > Well, this is not exactly right, at least this is suboptimal in that
> > other sub-threads can too call walk_process_tree(kill_descendant_visitor)
> > later for no reason.
>
> Oleg I think I am missing something.
No, it is stupid me who can't read,
> Reading kernel/exit.c I see "group_dead = atomic_dec_and_test(&tsk->signal->live)".
> Which seems like enough to ensure exactly one task/thread calls walk_process_tree.
Of course you right, sorry for confusion.
To me it would be more clean to call walk_process_tree(kill_descendant_visitor)
unconditionally in find_new_reaper() right before "if (has_child_subreaper)", but
then we will need to shift read_lock(tasklist) from walk_process_tree().
So I think the patch is mostly fine, the only problem I can see is that
PR_SET_KILL_DESCENDANTS_ON_EXIT can race with PR_SET_CHILD_SUBREAPER, they both
need to update the bits in the same word.
Oleg.
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Florian Weimer @ 2018-11-29 12:28 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181120105124.14733-1-christian@brauner.io>
Disclaimer: I'm looking at this patch because Christian requested it.
I'm not a kernel developer.
* Christian Brauner:
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..3f27ffd8ae87 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..8a30cde82450 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 64 procfd_signal __x64_sys_procfd_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> @@ -386,3 +387,4 @@
> 545 x32 execveat __x32_compat_sys_execveat/ptregs
> 546 x32 preadv2 __x32_compat_sys_preadv64v2
> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
Is there a reason why these numbers have to be different?
(See the recent discussion with Andy Lutomirski.)
> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t *kinfo, int flags,
> + bool had_siginfo)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + /* Is this a process file descriptor? */
> + ret = -EINVAL;
> + if (!proc_is_tgid_procfd(f.file))
> + goto err;
[…]
> + ret = kill_pid_info(sig, kinfo, pid);
I would like to see some comment here what happens to zombie processes.
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
Sorry, I'm quite unhappy with the name. “signal” is for signal handler
management. procfd_sendsignal, procfd_sigqueueinfo or something like
that would be fine. Even procfd_kill would be better IMHO.
Looking at the rt_tgsigqueueinfo interface, is there a way to implement
the “tg” part with the current procfd_signal interface? Would you use
openat to retrieve the Tgid: line from "status"?
Thanks,
Florian
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Florian Weimer @ 2018-11-29 11:37 UTC (permalink / raw)
To: Dave Hansen; +Cc: linux-mm, Ram Pai, linuxppc-dev, linux-api
In-Reply-To: <58e263a6-9a93-46d6-c5f9-59973064d55e@intel.com>
* Dave Hansen:
> On 11/27/18 3:57 AM, Florian Weimer wrote:
>> I would have expected something that translates PKEY_DISABLE_WRITE |
>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
>>
>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
>> access, but produces execute-only memory.)
>
> Correct, it disables all data access, but not execution.
So I would expect something like this (completely untested, I did not
even compile this):
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf153c871..bed23f9e8336 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -199,6 +199,11 @@ static inline bool arch_pkeys_enabled(void)
return !static_branch_likely(&pkey_disabled);
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ return (rights & ~(unsigned long)PKEY_ACCESS_MASK) == 0;
+}
+
extern void pkey_mm_init(struct mm_struct *mm);
extern bool arch_supports_pkeys(int cap);
extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f1b3be..e3e1d5a316e8 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -14,6 +14,17 @@ static inline bool arch_pkeys_enabled(void)
return boot_cpu_has(X86_FEATURE_OSPKE);
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ if (rights & ~(unsigned long)PKEY_ACCESS_MASK)
+ return false;
+ if (rights & PKEY_DISABLE_READ) {
+ /* x86 can only disable read access along with write access. */
+ return rights & (PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS);
+ }
+ return true;
+}
+
/*
* Try to dedicate one of the protection keys to be used as an
* execute-only protection key.
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 87a57b7642d3..b9b78145017f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -928,7 +928,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return -EINVAL;
/* Set the bits we need in PKRU: */
- if (init_val & PKEY_DISABLE_ACCESS)
+ if (init_val & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ))
+ /*
+ * arch_pkey_access_rights_valid checked that
+ * PKEY_DISABLE_READ is actually representable on x86
+ * (that is, it comes with PKEY_DISABLE_ACCESS or
+ * PKEY_DISABLE_WRITE).
+ */
new_pkru_bits |= PKRU_AD_BIT;
if (init_val & PKEY_DISABLE_WRITE)
new_pkru_bits |= PKRU_WD_BIT;
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 2955ba976048..2c330fabbe55 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
{
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ return false;
+}
+
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
#endif /* _LINUX_PKEYS_H */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6d331620b9e5..f4cefc3540df 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -597,7 +597,7 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
if (flags)
return -EINVAL;
/* check for unsupported init values */
- if (init_val & ~PKEY_ACCESS_MASK)
+ if (!arch_pkey_access_rights_valid(init_val))
return -EINVAL;
down_write(¤t->mm->mmap_sem);
Thanks,
Florian
^ permalink raw reply related
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-29 10:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, Kees Cook, Jann Horn, Michael Ellerman,
Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
Linux API, Ingo Molnar, strace-devel
In-Reply-To: <CALCETrVjMorQDsJzQGaREEB5_7B24ApWSoNQfRd3K3QNSGAyEA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]
On Wed, Nov 28, 2018 at 03:17:49PM -0800, Andy Lutomirski wrote:
> On Wed, Nov 28, 2018 at 2:11 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
> >
> > On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> > > On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > > > On 11/28, Dmitry V. Levin wrote:
> > > > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > > > On 11/28, Dmitry V. Levin wrote:
> > > > > > >
> > > > > > > +/*
> > > > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > > > + * to describe current syscall-stop.
> > > > > > > + *
> > > > > > > + * Values for these constants are chosen so that they do not appear
> > > > > > > + * in task->ptrace_message by other means.
> > > > > > > + */
> > > > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > > > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > > > > >
> > > > > > Again, I do not really understand the comment... Why should we care about
> > > > > > "do not appear in task->ptrace_message by other means" ?
> > > > > >
> > > > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > > > numbers, say, 1 and 2?
> > > > > >
> > > > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > > > anyway after wait(status).
> > > > >
> > > > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > > > during syscall stop is undefined, we need two different ptrace_message
> > > > > values that cannot be set by other ptrace events to enable reliable
> > > > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > > > other ptrace events, it would be hard for userspace to find out whether
> > > > > the kernel implements new semantics or not.
> > > >
> > > > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > > > returns EIO then it is not implemented?
> > >
> > > The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> > > PTRACE_GETEVENTMSG for syscall stops.
> > > My concern here is the PTRACE_GETEVENTMSG interface itself. If we use
> > > ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> > > PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> > > it should have clear semantics.
> >
> > Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
> > to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
> > one of the following approaches:
> >
> > 1. Do not document the values saved into ptrace_message during syscall
> > stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
> > leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
> > as undefined.
> >
> > 2. Document these values chosen to avoid collisions with ptrace_message values
> > set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
> > whether this new semantics is supported by the kernel or not.
>
> I don't like any of this at all. Can we please choose a sensible API
> design and let the API drive the implementation instead of vice versa?
What are your concerns? Do you see something wrong in exposing this
information via PTRACE_GETEVENTMSG?
Anyway, can we agree on the PTRACE_GET_SYSCALL_INFO API, please?
> ISTM the correct solution is to add some new state to task_struct for
> this.
>
> If we're concerned about making task_struct bigger, I have a
> half-finished patch to factor all the ptrace tracee state into a
> separate struct.
This is refactoring of the kernel - a thing userspace people are not
the best equipped to do. This part should rather be sorted out by kernel
people.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Andy Lutomirski @ 2018-11-28 23:17 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Kees Cook, Jann Horn, Michael Ellerman,
Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
Andrew Lutomirski, Linux API, Ingo Molnar, strace-devel
In-Reply-To: <20181128221125.GA2800@altlinux.org>
On Wed, Nov 28, 2018 at 2:11 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> > On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > > On 11/28, Dmitry V. Levin wrote:
> > > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > > On 11/28, Dmitry V. Levin wrote:
> > > > > >
> > > > > > +/*
> > > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > > + * to describe current syscall-stop.
> > > > > > + *
> > > > > > + * Values for these constants are chosen so that they do not appear
> > > > > > + * in task->ptrace_message by other means.
> > > > > > + */
> > > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > > > >
> > > > > Again, I do not really understand the comment... Why should we care about
> > > > > "do not appear in task->ptrace_message by other means" ?
> > > > >
> > > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > > numbers, say, 1 and 2?
> > > > >
> > > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > > anyway after wait(status).
> > > >
> > > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > > during syscall stop is undefined, we need two different ptrace_message
> > > > values that cannot be set by other ptrace events to enable reliable
> > > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > > other ptrace events, it would be hard for userspace to find out whether
> > > > the kernel implements new semantics or not.
> > >
> > > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > > returns EIO then it is not implemented?
> >
> > The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> > PTRACE_GETEVENTMSG for syscall stops.
> > My concern here is the PTRACE_GETEVENTMSG interface itself. If we use
> > ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> > PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> > it should have clear semantics.
>
> Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
> to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
> one of the following approaches:
>
> 1. Do not document the values saved into ptrace_message during syscall
> stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
> leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
> as undefined.
>
> 2. Document these values chosen to avoid collisions with ptrace_message values
> set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
> whether this new semantics is supported by the kernel or not.
I don't like any of this at all. Can we please choose a sensible API
design and let the API drive the implementation instead of vice versa?
ISTM the correct solution is to add some new state to task_struct for
this.
If we're concerned about making task_struct bigger, I have a
half-finished patch to factor all the ptrace tracee state into a
separate struct.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Joey Pabalinas @ 2018-11-28 23:02 UTC (permalink / raw)
To: Christian Brauner
Cc: Joey Pabalinas, ebiederm, linux-kernel, serge, jannh, luto, akpm,
oleg, cyphar, viro, linux-fsdevel, linux-api, dancol, timmurray,
linux-man, Kees Cook
In-Reply-To: <20181128220545.fplqmbkfhxowx3lf@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
On Wed, Nov 28, 2018 at 11:05:49PM +0100, Christian Brauner wrote:
> On Wed, Nov 28, 2018 at 11:45:34AM -1000, Joey Pabalinas wrote:
> > On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> > > + if (info) {
> > > + ret = __copy_siginfo_from_user(sig, &kinfo, info);
> > > + if (unlikely(ret))
> > > + goto err;
> >
>
> I think you're misreading here. It jumps to:
>
> err:
> fdput(f);
> return ret;
>
> and does propagate the error. This is also an old iteration of the patch.
Whoops, that I am. Sorry about that.
--
Cheers,
Joey Pabalinas
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
From: Dmitry V. Levin @ 2018-11-28 22:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Jann Horn, Michael Ellerman, Eugene Syromyatnikov,
Steven Rostedt, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181128152346.GG28206-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 3344 bytes --]
On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > On 11/28, Dmitry V. Levin wrote:
> > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > On 11/28, Dmitry V. Levin wrote:
> > > > >
> > > > > +/*
> > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > + * to describe current syscall-stop.
> > > > > + *
> > > > > + * Values for these constants are chosen so that they do not appear
> > > > > + * in task->ptrace_message by other means.
> > > > > + */
> > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 0x80000000U
> > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > > >
> > > > Again, I do not really understand the comment... Why should we care about
> > > > "do not appear in task->ptrace_message by other means" ?
> > > >
> > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > numbers, say, 1 and 2?
> > > >
> > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > anyway after wait(status).
> > >
> > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > during syscall stop is undefined, we need two different ptrace_message
> > > values that cannot be set by other ptrace events to enable reliable
> > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > other ptrace events, it would be hard for userspace to find out whether
> > > the kernel implements new semantics or not.
> >
> > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > returns EIO then it is not implemented?
>
> The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> PTRACE_GETEVENTMSG for syscall stops.
> My concern here is the PTRACE_GETEVENTMSG interface itself. If we use
> ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> it should have clear semantics.
Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
one of the following approaches:
1. Do not document the values saved into ptrace_message during syscall
stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
as undefined.
2. Document these values chosen to avoid collisions with ptrace_message values
set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
whether this new semantics is supported by the kernel or not.
The first approach was implemented in v2 of this series: the constants
were PT_SYSCALL_IS_{ENTERING,EXITING} defined in include/linux/ptrace.h.
The second approach was implemented in v3: the constants are
PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} defined in include/uapi/linux/ptrace.h,
they are also going to be documented in ptrace(2) man page.
Since the use of ptrace_message is exposed to PTRACE_GETEVENTMSG users
anyway, I do not see any reason to choose the first approach over the
second.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-28 22:05 UTC (permalink / raw)
To: Joey Pabalinas
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181128214534.xxqoyy7mi5dw54kj@gmail.com>
On Wed, Nov 28, 2018 at 11:45:34AM -1000, Joey Pabalinas wrote:
> On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> > + if (info) {
> > + ret = __copy_siginfo_from_user(sig, &kinfo, info);
> > + if (unlikely(ret))
> > + goto err;
>
I think you're misreading here. It jumps to:
err:
fdput(f);
return ret;
and does propagate the error. This is also an old iteration of the patch.
Christian
> What's the reason you don't propagate up the errors from __copy_siginfo_from_user()?
> Granted, I admit that -E2BIG is kind of weird to return, but -EFAULT seems like a
> fairly sane error.
>
> Or is there some reason it's more useful to just return -EINVAL for all of the
> failure cases here?
>
> --
> Cheers,
> Joey Pabalinas
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Joey Pabalinas @ 2018-11-28 21:45 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook, Joey Pabalinas
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 510 bytes --]
On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> + if (info) {
> + ret = __copy_siginfo_from_user(sig, &kinfo, info);
> + if (unlikely(ret))
> + goto err;
What's the reason you don't propagate up the errors from __copy_siginfo_from_user()?
Granted, I admit that -E2BIG is kind of weird to return, but -EFAULT seems like a
fairly sane error.
Or is there some reason it's more useful to just return -EINVAL for all of the
failure cases here?
--
Cheers,
Joey Pabalinas
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] procfd_signal.2: document procfd_signal syscall
From: Christian Brauner @ 2018-11-28 21:12 UTC (permalink / raw)
To: Florian Weimer
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man
In-Reply-To: <87in0g7waf.fsf@oldenburg.str.redhat.com>
On November 29, 2018 9:59:52 AM GMT+13:00, Florian Weimer <fweimer@redhat.com> wrote:
>* Christian Brauner:
>
>> +.\" Copyright (C) 2018 Christian Brauner <christian@brauner.io>
>
>The text seems to be largely derived from rt_sigqueueinfo, so I'm not
>sure if this appropriate here.
>
>> +the null signal (0) can be used to check if a process with a given
>> +PID exists.
>
>What does this mean if hte process is identified by file descriptor?
>
>> +.PP
>> +The optional
>> +.I info
>> +argument specifies the data to accompany the signal.
>> +This argument is a pointer to a structure of type
>> +.IR siginfo_t ,
>> +described in
>> +.BR sigaction (2)
>> +(and defined by including
>> +.IR <sigaction.h> ).
>> +The caller should set the following fields in this structure:
>> +.TP
>> +.I si_code
>> +This must be one of the
>> +.B SI_*
>> +codes in the Linux kernel source file
>> +.IR include/asm-generic/siginfo.h ,
>> +with the restriction that the code must be negative
>> +(i.e., cannot be
>> +.BR SI_USER ,
>> +which is used by the kernel to indicate a signal sent by
>> +.BR kill (2))
>> +and cannot (since Linux 2.6.39) be
>
>Obsolete reference in this context.
>
>> +.TP
>> +.B ESRCH
>> +The process or process group does not exist.
>> +Note that an existing process might be a zombie,
>> +a process that has terminated execution, but
>> +has not yet been
>> +.BR wait (2)ed
>> +for.
>
>Again: What does this mean if the process identified by a descriptor?
>Does a process in zombie state exist in this sense or not?
>
>Thanks,
>Florian
Updating the document is on my Todo.
Florian, can you take a look at the actual patch too, please?
^ permalink raw reply
* Re: [PATCH] procfd_signal.2: document procfd_signal syscall
From: Florian Weimer @ 2018-11-28 20:59 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man
In-Reply-To: <20181119103241.5229-4-christian@brauner.io>
* Christian Brauner:
> +.\" Copyright (C) 2018 Christian Brauner <christian@brauner.io>
The text seems to be largely derived from rt_sigqueueinfo, so I'm not
sure if this appropriate here.
> +the null signal (0) can be used to check if a process with a given
> +PID exists.
What does this mean if hte process is identified by file descriptor?
> +.PP
> +The optional
> +.I info
> +argument specifies the data to accompany the signal.
> +This argument is a pointer to a structure of type
> +.IR siginfo_t ,
> +described in
> +.BR sigaction (2)
> +(and defined by including
> +.IR <sigaction.h> ).
> +The caller should set the following fields in this structure:
> +.TP
> +.I si_code
> +This must be one of the
> +.B SI_*
> +codes in the Linux kernel source file
> +.IR include/asm-generic/siginfo.h ,
> +with the restriction that the code must be negative
> +(i.e., cannot be
> +.BR SI_USER ,
> +which is used by the kernel to indicate a signal sent by
> +.BR kill (2))
> +and cannot (since Linux 2.6.39) be
Obsolete reference in this context.
> +.TP
> +.B ESRCH
> +The process or process group does not exist.
> +Note that an existing process might be a zombie,
> +a process that has terminated execution, but
> +has not yet been
> +.BR wait (2)ed
> +for.
Again: What does this mean if the process identified by a descriptor?
Does a process in zombie state exist in this sense or not?
Thanks,
Florian
^ permalink raw reply
* Re: extending wait4(2) or waitid(2) linux syscall
From: Daniel Colascione @ 2018-11-28 18:50 UTC (permalink / raw)
To: Florian Weimer
Cc: Christian Brauner, GNU C Library, Arnd Bergmann, Dmitry V. Levin,
Albert ARIBAUD, H. Peter Anvin, Linux API
In-Reply-To: <87efb5h74j.fsf@oldenburg.str.redhat.com>
On Wed, Nov 28, 2018 at 1:41 AM, Florian Weimer <fweimer@redhat.com> wrote:
> * Christian Brauner:
>
>> The intention has always been to start a
>> file descriptor process API off of that.
>> If we land my procfd_signal() patchset we are in good shape for
>> procfd_wait(), imho.
>
> How does this interact with SIGCHLD and the wait system call (or any
> wait function without an explicitly specified PID)?
It doesn't, but let's take one step at a time. procfd_signal() is a
good first step. Once procfd_signal() and procfd_wait() exist, and
managing processes with FDs in general no longer seems odd, we can add
a clone() option that 1) avoid wiring a new subprocess up to the
legacy PID-based process-management API, and 2) returns one of these
fancy new FDs that works with procfd_signal and procfd_wait. (How we
do that without /proc mounted is an open question, but solvable in a
few options.)
> I understand that I have somewhat conflicting requirements, but in terms
> of relative priorities, launching a process without spurious signals and
> wait notifications would probably offer the larger benefit.
I also want a non-process-wide subprocess API. A clean FD-based API is
how you get it--- it's not an either-or situation. Considering that we
want to use FDs for general-purpose process management in the future,
it'd be not only a waste of time, but actually counterproductive, to
design a non-FD-based API for this non-process-wide sub-process use
case.
^ permalink raw reply
* [PATCH v4 4/4] selftests: add a test for bpf_prog_test_run_xattr
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181128165312.19500-1-lmb@cloudflare.com>
Make sure that bpf_prog_test_run_xattr returns the correct length
and that the kernel respects the output size hint. Also check
that errno indicates ENOSPC if there is a short output buffer given.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_progs.c | 55 +++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..ee827deb948a 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -70,7 +70,7 @@ static struct {
.tcp.urg_ptr = 123,
};
-#define CHECK(condition, tag, format...) ({ \
+#define _CHECK(condition, tag, duration, format...) ({ \
int __ret = !!(condition); \
if (__ret) { \
error_cnt++; \
@@ -83,6 +83,11 @@ static struct {
__ret; \
})
+#define CHECK(condition, tag, format...) \
+ _CHECK(condition, tag, duration, format)
+#define CHECK_ATTR(condition, tag, format...) \
+ _CHECK(condition, tag, tattr.duration, format)
+
static int bpf_find_map(const char *test, struct bpf_object *obj,
const char *name)
{
@@ -124,6 +129,53 @@ static void test_pkt_access(void)
bpf_object__close(obj);
}
+static void test_prog_run_xattr(void)
+{
+ const char *file = "./test_pkt_access.o";
+ struct bpf_object *obj;
+ char buf[10];
+ int err;
+ struct bpf_prog_test_run_attr tattr = {
+ .repeat = 1,
+ .data_in = &pkt_v4,
+ .data_size_in = sizeof(pkt_v4),
+ .data_out = buf,
+ .data_size_out = 5,
+ };
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj,
+ &tattr.prog_fd);
+ if (CHECK_ATTR(err, "load", "err %d errno %d\n", err, errno))
+ return;
+
+ memset(buf, 0, sizeof(buf));
+
+ err = bpf_prog_test_run_xattr(&tattr);
+ CHECK_ATTR(err != -1 || errno != ENOSPC || tattr.retval, "run",
+ "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+ CHECK_ATTR(tattr.data_size_out != sizeof(pkt_v4), "data_size_out",
+ "incorrect output size, want %lu have %u\n",
+ sizeof(pkt_v4), tattr.data_size_out);
+
+ CHECK_ATTR(buf[5] != 0, "overflow",
+ "BPF_PROG_TEST_RUN ignored size hint\n");
+
+ tattr.data_out = NULL;
+ tattr.data_size_out = 0;
+ errno = 0;
+
+ err = bpf_prog_test_run_xattr(&tattr);
+ CHECK_ATTR(err || errno || tattr.retval, "run_no_output",
+ "err %d errno %d retval %d\n", err, errno, tattr.retval);
+
+ tattr.data_size_out = 1;
+ err = bpf_prog_test_run_xattr(&tattr);
+ CHECK_ATTR(err != -EINVAL, "run_wrong_size_out", "err %d\n", err);
+
+ bpf_object__close(obj);
+}
+
static void test_xdp(void)
{
struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1837,6 +1889,7 @@ int main(void)
jit_enabled = is_jit_enabled();
test_pkt_access();
+ test_prog_run_xattr();
test_xdp();
test_xdp_adjust_tail();
test_l4lb_all();
--
2.17.1
^ permalink raw reply related
* [PATCH v4 3/4] libbpf: add bpf_prog_test_run_xattr
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181128165312.19500-1-lmb@cloudflare.com>
Add a new function, which encourages safe usage of the test interface.
bpf_prog_test_run continues to work as before, but should be considered
unsafe.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/lib/bpf/bpf.c | 23 +++++++++++++++++++++++
tools/lib/bpf/bpf.h | 19 +++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index ce1822194590..6e06484a7a99 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -463,6 +463,29 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
return ret;
}
+int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr)
+{
+ union bpf_attr attr;
+ int ret;
+
+ if (!test_attr->data_out && test_attr->data_size_out > 0)
+ return -EINVAL;
+
+ bzero(&attr, sizeof(attr));
+ attr.test.prog_fd = test_attr->prog_fd;
+ attr.test.data_in = ptr_to_u64(test_attr->data_in);
+ attr.test.data_out = ptr_to_u64(test_attr->data_out);
+ attr.test.data_size_in = test_attr->data_size_in;
+ attr.test.data_size_out = test_attr->data_size_out;
+ attr.test.repeat = test_attr->repeat;
+
+ ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+ test_attr->data_size_out = attr.test.data_size_out;
+ test_attr->retval = attr.test.retval;
+ test_attr->duration = attr.test.duration;
+ return ret;
+}
+
int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
{
union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 09e8bbe111d4..ce6f07399d41 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -118,6 +118,25 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+ int prog_fd;
+ int repeat;
+ const void *data_in;
+ __u32 data_size_in;
+ void *data_out; /* optional */
+ __u32 data_size_out; /* in: max length of data_out
+ * out: length of data_out */
+ __u32 retval; /* out: return code of the BPF program */
+ __u32 duration; /* out: average per repetition in ns */
+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
+
+/*
+ * bpf_prog_test_run does not check that data_out is large enough. Consider
+ * using bpf_prog_test_run_xattr instead.
+ */
LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
__u32 size, void *data_out, __u32 *size_out,
__u32 *retval, __u32 *duration);
--
2.17.1
^ permalink raw reply related
* [PATCH v4 2/4] tools: sync uapi/linux/bpf.h
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181128165312.19500-1-lmb@cloudflare.com>
Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
--
2.17.1
^ permalink raw reply related
* [PATCH v4 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Lorenz Bauer @ 2018-11-28 16:53 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181128165312.19500-1-lmb@cloudflare.com>
Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 7 +++++--
net/bpf/test_run.c | 15 +++++++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 23e2031a43d4..01b0e5f485c4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -360,8 +360,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
{
void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
int err = -EFAULT;
+ u32 copy_size = size;
- if (data_out && copy_to_user(data_out, data, size))
+ /* Clamp copy if the user has provided a size hint, but copy the full
+ * buffer if not to retain old behaviour.
+ */
+ if (kattr->test.data_size_out &&
+ copy_size > kattr->test.data_size_out) {
+ copy_size = kattr->test.data_size_out;
+ err = -ENOSPC;
+ }
+
+ if (data_out && copy_to_user(data_out, data, copy_size))
goto out;
if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
goto out;
if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
goto out;
- err = 0;
+ if (err != -ENOSPC)
+ err = 0;
out:
return err;
}
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox