* Re: [PATCH v3 0/3] namei: implement various lookup restriction AT_* flags
From: Aleksa Sarai @ 2018-10-17 15:23 UTC (permalink / raw)
To: Al Viro, Eric Biederman
Cc: Andy Lutomirski, David Howells, Jann Horn, Christian Brauner,
David Drysdale, containers, linux-fsdevel, linux-api, Jeff Layton,
J. Bruce Fields, Arnd Bergmann, Tycho Andersen, dev, linux-kernel,
linux-arch
In-Reply-To: <20181009070230.12884-1-cyphar@cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]
On 2018-10-09, Aleksa Sarai <cyphar@cyphar.com> wrote:
> The need for some sort of control over VFS's path resolution (to avoid
> malicious paths resulting in inadvertent breakouts) has been a very
> long-standing desire of many userspace applications. This patchset is a
> revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
> of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
> Capsicum project[4]) with a few additions and changes made based on the
> previous discussion within [5] as well as others I felt were useful.
I've been thinking about this problem a little more (from the UX side of
things) and I have a feeling that adding 5 different O_* flags related
to resolution -- rather than properties related to opening the file --
might be less than ideal (even though, as discussed in previous threads,
there is a need for these flags and for them to be separated).
There is *some* precedence for this with O_PATH[**] changing fairly
large semantics of openat(2) but there are some things about O_PATH
which I think could be improved.
What if we had a resolveat(2) which acted like openat(..., O_PATH) *but*
it allowed us to have new flags and to separate the scoping flags from
the (fairly limited) space of O_* flags. Then O_PATH could effectively
just be a legacy way of doing resolveat(2) -- with only O_CLOEXEC,
O_DIRECTORY, and O_NOFOLLOW support.
And the main things we could add would be:
* These resolution flags, with only support available from
resolveat(2) for the moment. The idea would be that AT_EMPTY_PATH
would be the recommended way to make use of this.
* Support for RESOLVE_{NOPERM,RDONLY,WRONLY,RDWR} (which after some
discussions with Eric last year might be necessary in order to make
/proc/$pid/fd/$fd re-opening of O_PATH descriptors safer -- which is
something that we use in both runc and LXC).
Is this idea palatable, or was this something considered during the
development of O_PATH and someone had an argument why augmenting O_PATH
is better than a new syscall?
[**] And while writing this paragraph I noticed that I didn't update the
O_PATH "flag whitelist" to allow the scoping flags to affect it. I
will include a fix for this in v4 (I must've lost it in an early
rebase before I sent v1).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Mathieu Desnoyers @ 2018-10-17 15:11 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <20181017071931.GA18379@linux.vnet.ibm.com>
----- On Oct 17, 2018, at 3:19 AM, Srikar Dronamraju srikar@linux.vnet.ibm.com wrote:
> Hi Mathieu,
>
>> +static int do_cpu_opv(struct cpu_op *cpuop, int cpuopcnt,
>> + struct cpu_opv_vaddr *vaddr_ptrs, int cpu)
>> +{
>> + struct mm_struct *mm = current->mm;
>> + int ret;
>> +
>> +retry:
>> + if (cpu != raw_smp_processor_id()) {
>> + ret = push_task_to_cpu(current, cpu);
>> + if (ret)
>> + goto check_online;
>> + }
>> + down_read(&mm->mmap_sem);
>> + ret = vaddr_ptrs_check(vaddr_ptrs);
>> + if (ret)
>> + goto end;
>> + preempt_disable();
>> + if (cpu != smp_processor_id()) {
>> + preempt_enable();
>> + up_read(&mm->mmap_sem);
>> + goto retry;
>> + }
>
> If we have a higher priority task/s either pinned to the cpu, dont we end up
> in busy-looping till the task exits/sleeps?
You're right!
How about we ditch the thread migration altogether, and simply perform
the cpu_opv operations in a IPI handler ?
This is possible now that cpu_opv uses a temporary vmap() rather than
try to touch the user-space page through the current thread's page table.
Thoughts ?
Thanks,
Mathieu
>
>> + ret = __do_cpu_opv(cpuop, cpuopcnt);
>> + preempt_enable();
>> +end:
>> + up_read(&mm->mmap_sem);
>> + return ret;
>> +
>> +check_online:
>> + /*
>> + * push_task_to_cpu() returns -EINVAL if the requested cpu is not part
>> + * of the current thread's cpus_allowed mask.
>> + */
>> + if (ret == -EINVAL)
>> + return ret;
>> + get_online_cpus();
>> + if (cpu_online(cpu)) {
>> + put_online_cpus();
>> + goto retry;
> > + }
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.21 03/16] sched: Implement push_task_to_cpu (v2)
From: Mathieu Desnoyers @ 2018-10-17 15:09 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <20181017065145.GA7111@linux.vnet.ibm.com>
----- On Oct 17, 2018, at 2:51 AM, Srikar Dronamraju srikar@linux.vnet.ibm.com wrote:
> Hi Mathieu,
>
>> +int push_task_to_cpu(struct task_struct *p, unsigned int dest_cpu)
>> +{
>
> In your use case, is the task going to be current?
> If yes, we should simply be using migrate_task_to.
>
>> + struct rq_flags rf;
>> + struct rq *rq;
>> + int ret = 0;
>> +
>> + rq = task_rq_lock(p, &rf);
>> + update_rq_clock(rq);
>> +
>> + if (!cpumask_test_cpu(dest_cpu, &p->cpus_allowed)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>
> Ideally we should have checked cpus_allowed/cpu_active_mask before taking
> the lock. This would help reduce the contention on the rqlock when the
> passed parameter is not correct.
>
>> +
>> + if (!cpumask_test_cpu(dest_cpu, cpu_active_mask)) {
>> + ret = -EBUSY;
>> + goto out;
>> + }
>> +
>> + if (task_cpu(p) == dest_cpu)
>> + goto out;
>
> Same as above.
>
>> +
>> + if (task_running(rq, p) || p->state == TASK_WAKING) {
>
> Why are we using migration thread to move a task in TASK_WAKING state?
>
>> + struct migration_arg arg = { p, dest_cpu };
>> + /* Need help from migration thread: drop lock and wait. */
>> + task_rq_unlock(rq, p, &rf);
>> + stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
>> + tlb_migrate_finish(p->mm);
>> + return 0;
>
> Why cant we use migrate_task_to instead?
I could do that be moving migrate_task_to outside of NUMA-specific #ifdef,
but I think we can do much, much simpler than that, see below.
>
>> + } else if (task_on_rq_queued(p)) {
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index 455fa330de04..27ad25780204 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -1340,6 +1340,15 @@ static inline void __set_task_cpu(struct task_struct *p,
>> unsigned int cpu)
>> #endif
>> }
>>
>> +#ifdef CONFIG_SMP
>> +int push_task_to_cpu(struct task_struct *p, unsigned int dest_cpu);
>> +#else
>> +static inline int push_task_to_cpu(struct task_struct *p, unsigned int
>> dest_cpu)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>
> Your usecase is outside kernel/sched. So I am not sure if this is the right
> place for the declaration.
Actually, now that I think of it, we may not need to migrate the task at all.
Now that cpu_opv implementation takes a temporary vmap() of the user-space pages,
we can touch that virtual address range from interrupt context from another CPU.
So cpu_opv can simply execute the vector of operations in IPI context rather than
do all this silly dance with migration.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Mathieu Desnoyers @ 2018-10-17 15:04 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: rostedt, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
linux-kernel, linux-api, Thomas Gleixner, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <1051015468.759.1539788401096.JavaMail.zimbra@efficios.com>
----- On Oct 17, 2018, at 11:00 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Oct 16, 2018, at 8:27 PM, Sergey Senozhatsky
> sergey.senozhatsky.work@gmail.com wrote:
>
>> On (10/16/18 14:30), Steven Rostedt wrote:
>>> > +void vm_unmap_user_ram(const void *mem, unsigned int count)
>>> > +{
>>> > + unsigned long size = (unsigned long)count << PAGE_SHIFT;
>>> > + unsigned long addr = (unsigned long)mem;
>>> > + struct vmap_area *va;
>>> > +
>>> > + might_sleep();
>>> > + BUG_ON(!addr);
>>> > + BUG_ON(addr < VMALLOC_START);
>>> > + BUG_ON(addr > VMALLOC_END);
>>> > + BUG_ON(!PAGE_ALIGNED(addr));
>>> > +
>>> > + debug_check_no_locks_freed(mem, size);
>>> > + va = find_vmap_area(addr);
>>> > + BUG_ON(!va);
>>> > + free_unmap_vmap_area(va);
>>> > +}
>>> > +EXPORT_SYMBOL(vm_unmap_user_ram);
>>> > +
>>>
>>> Noticing this from Sergey's question in another patch, why are you
>>> using BUG_ON()? That's rather extreme and something we are trying to
>>> avoid adding more of (I still need to remove the BUG_ON()s I've added
>>> over ten years ago). I don't see why all these BUG_ON's can't be turned
>>> into:
>>
>> +1
>>
>>> if (WARN_ON(x))
>>> return;
>>
>> Given that this somewhat MM-related, I'd may be say VM_WARN_ON().
I notice that VM_WARN_ON() casts the result of WARN_ON() to (void), so it
cannot be used in a if () statement.
VM_WARN_ON() will only warn if CONFIG_DEBUG_VM is set.
Is it really what we want ?
Thanks,
Mathieu
>
> Good point, will do!
>
> So I'll do one cleanup patch for vm_unmap_ram(), and I'll modify the new
> vm_unmap_user_ram().
>
> Thanks,
>
> Mathieu
>
>>
>> -ss
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Mathieu Desnoyers @ 2018-10-17 15:00 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: rostedt, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
linux-kernel, linux-api, Thomas Gleixner, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <20181017002722.GA1068@jagdpanzerIV>
----- On Oct 16, 2018, at 8:27 PM, Sergey Senozhatsky sergey.senozhatsky.work@gmail.com wrote:
> On (10/16/18 14:30), Steven Rostedt wrote:
>> > +void vm_unmap_user_ram(const void *mem, unsigned int count)
>> > +{
>> > + unsigned long size = (unsigned long)count << PAGE_SHIFT;
>> > + unsigned long addr = (unsigned long)mem;
>> > + struct vmap_area *va;
>> > +
>> > + might_sleep();
>> > + BUG_ON(!addr);
>> > + BUG_ON(addr < VMALLOC_START);
>> > + BUG_ON(addr > VMALLOC_END);
>> > + BUG_ON(!PAGE_ALIGNED(addr));
>> > +
>> > + debug_check_no_locks_freed(mem, size);
>> > + va = find_vmap_area(addr);
>> > + BUG_ON(!va);
>> > + free_unmap_vmap_area(va);
>> > +}
>> > +EXPORT_SYMBOL(vm_unmap_user_ram);
>> > +
>>
>> Noticing this from Sergey's question in another patch, why are you
>> using BUG_ON()? That's rather extreme and something we are trying to
>> avoid adding more of (I still need to remove the BUG_ON()s I've added
>> over ten years ago). I don't see why all these BUG_ON's can't be turned
>> into:
>
> +1
>
>> if (WARN_ON(x))
>> return;
>
> Given that this somewhat MM-related, I'd may be say VM_WARN_ON().
Good point, will do!
So I'll do one cleanup patch for vm_unmap_ram(), and I'll modify the new vm_unmap_user_ram().
Thanks,
Mathieu
>
> -ss
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Alan Jenkins @ 2018-10-17 14:55 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Howells, Al Viro, linux-api, torvalds, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <87woqgiqta.fsf@xmission.com>
On 17/10/2018 15:35, Eric W. Biederman wrote:
> Alan Jenkins <alan.christopher.jenkins@gmail.com> writes:
>
>> On 17/10/2018 14:20, David Howells wrote:
>>> David Howells <dhowells@redhat.com> wrote:
>>>
>>>> I should probably check that the picked point is actually a mountpoint.
>>> The root of the mount object at the path specified, that is, perhaps with
>>> something like the attached.
>>>
>>> David
>>
>> I agree. I'm happy to see this is using the same check as do_remount().
>>
>>
>> * change filesystem flags. dir should be a physical root of filesystem.
>> * If you've mounted a non-root directory somewhere and want to do remount
>> * on it - tough luck.
>> */
> Davids check will work for bind mounts as well. It just won't work it
> just won't work for files or subdirectories of some mountpoint.
>
> Eric
I see. Then I am still happy to see the fspick() check match a check in
do_remount() (and it still solves the problem I was worried about).
I cannot blame David for the do_remount() comment being out of date :-).
# uname -r
4.18.10-200.fc.28.x86_64
# mount --bind /mnt /mnt
# mount -oremount,debug /mnt
# findmnt /mnt; findmnt /
[findmnt shows / has been remounted, adding the ext4 "debug" mount option]
>
>>> ---
>>> diff --git a/fs/fsopen.c b/fs/fsopen.c
>>> index f673e93ac456..aaaaa17a233c 100644
>>> --- a/fs/fsopen.c
>>> +++ b/fs/fsopen.c
>>> @@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
>>> if (ret < 0)
>>> goto err;
>>> + ret = -EINVAL;
>>> + if (target.mnt->mnt_root != target.dentry)
>>> + goto err_path;
>>> +
>>> fc = vfs_new_fs_context(target.dentry->d_sb->s_type, target.dentry,
>>> 0, 0, FS_CONTEXT_FOR_RECONFIGURE);
>>> if (IS_ERR(fc)) {
>>>
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Eric W. Biederman @ 2018-10-17 14:35 UTC (permalink / raw)
To: Alan Jenkins
Cc: David Howells, Al Viro, linux-api, torvalds, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <f6f2553e-f64d-9c26-4290-baf43e5f533b@gmail.com>
Alan Jenkins <alan.christopher.jenkins@gmail.com> writes:
> On 17/10/2018 14:20, David Howells wrote:
>> David Howells <dhowells@redhat.com> wrote:
>>
>>> I should probably check that the picked point is actually a mountpoint.
>> The root of the mount object at the path specified, that is, perhaps with
>> something like the attached.
>>
>> David
>
>
> I agree. I'm happy to see this is using the same check as do_remount().
>
>
> * change filesystem flags. dir should be a physical root of filesystem.
> * If you've mounted a non-root directory somewhere and want to do remount
> * on it - tough luck.
> */
Davids check will work for bind mounts as well. It just won't work it
just won't work for files or subdirectories of some mountpoint.
Eric
>> ---
>> diff --git a/fs/fsopen.c b/fs/fsopen.c
>> index f673e93ac456..aaaaa17a233c 100644
>> --- a/fs/fsopen.c
>> +++ b/fs/fsopen.c
>> @@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
>> if (ret < 0)
>> goto err;
>> + ret = -EINVAL;
>> + if (target.mnt->mnt_root != target.dentry)
>> + goto err_path;
>> +
>> fc = vfs_new_fs_context(target.dentry->d_sb->s_type, target.dentry,
>> 0, 0, FS_CONTEXT_FOR_RECONFIGURE);
>> if (IS_ERR(fc)) {
>>
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: Alan Jenkins @ 2018-10-17 14:31 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <28710.1539782412@warthog.procyon.org.uk>
On 17/10/2018 14:20, David Howells wrote:
> David Howells <dhowells@redhat.com> wrote:
>
>> I should probably check that the picked point is actually a mountpoint.
> The root of the mount object at the path specified, that is, perhaps with
> something like the attached.
>
> David
I agree. I'm happy to see this is using the same check as do_remount().
* change filesystem flags. dir should be a physical root of filesystem.
* If you've mounted a non-root directory somewhere and want to do remount
* on it - tough luck.
*/
Thanks
Alan
> ---
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index f673e93ac456..aaaaa17a233c 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
> if (ret < 0)
> goto err;
>
> + ret = -EINVAL;
> + if (target.mnt->mnt_root != target.dentry)
> + goto err_path;
> +
> fc = vfs_new_fs_context(target.dentry->d_sb->s_type, target.dentry,
> 0, 0, FS_CONTEXT_FOR_RECONFIGURE);
> if (IS_ERR(fc)) {
>
^ permalink raw reply
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: David Howells @ 2018-10-17 13:20 UTC (permalink / raw)
Cc: dhowells, Alan Jenkins, Al Viro, linux-api, torvalds, ebiederm,
linux-fsdevel, linux-kernel, mszeredi
In-Reply-To: <19866.1539782101@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> wrote:
> I should probably check that the picked point is actually a mountpoint.
The root of the mount object at the path specified, that is, perhaps with
something like the attached.
David
---
diff --git a/fs/fsopen.c b/fs/fsopen.c
index f673e93ac456..aaaaa17a233c 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -186,6 +186,10 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
if (ret < 0)
goto err;
+ ret = -EINVAL;
+ if (target.mnt->mnt_root != target.dentry)
+ goto err_path;
+
fc = vfs_new_fs_context(target.dentry->d_sb->s_type, target.dentry,
0, 0, FS_CONTEXT_FOR_RECONFIGURE);
if (IS_ERR(fc)) {
^ permalink raw reply related
* Re: [PATCH 31/34] vfs: syscall: Add fspick() to select a superblock for reconfiguration [ver #12]
From: David Howells @ 2018-10-17 13:15 UTC (permalink / raw)
To: Alan Jenkins
Cc: dhowells, Al Viro, linux-api, torvalds, ebiederm, linux-fsdevel,
linux-kernel, mszeredi
In-Reply-To: <68a2107f-bf70-055b-86cf-1ba2ba9422bf@gmail.com>
Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
> Sorry, I guess SB_NOUSER was the wrong word. I was trying find if anything
> stopped things like
>
> int memfd = memfd_create("foo", 0);
> int fsfd = fspick(memfd, "", FSPICK_EMPTY_PATH);
>
> fsconfig(fsfd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
> fsconfig(fsfd, FSCONFIG_SET_STRING, "size", "100M", 0);
> fsconfig(fsfd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);
>
> So far I'm getting -EBUSY if I try to apply the "ro", -EINVAL if I try to
> apply the "size=100M". But if I don't apply either, then
> FSCONFIG_CMD_RECONFIGURE succeeds.
I should probably check that the picked point is actually a mountpoint.
David
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Borislav Petkov @ 2018-10-17 10:41 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <20181011151523.27101-4-yu-cheng.yu@intel.com>
On Thu, Oct 11, 2018 at 08:14:59AM -0700, Yu-cheng Yu wrote:
> Control Flow Enforcement (CET) MSRs are XSAVES system states.
That sentence needs massaging. MSRs are system states?!?!
> To support CET, we introduce XSAVES system states first.
Pls drop the "we" in all commit messages and convert the tone to
impartial and passive.
Also, this commit message needs to explain *why* you're doing this - it
is too laconic.
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/fpu/internal.h | 3 +-
> arch/x86/include/asm/fpu/xstate.h | 4 +-
> arch/x86/kernel/fpu/core.c | 6 +-
> arch/x86/kernel/fpu/init.c | 10 ----
> arch/x86/kernel/fpu/xstate.c | 86 ++++++++++++++++++-----------
> 5 files changed, 62 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 02c4296478c8..9a5db5a63f60 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -45,7 +45,6 @@ extern void fpu__init_cpu_xstate(void);
> extern void fpu__init_system(struct cpuinfo_x86 *c);
> extern void fpu__init_check_bugs(void);
> extern void fpu__resume_cpu(void);
> -extern u64 fpu__get_supported_xfeatures_mask(void);
>
> /*
> * Debugging facility:
> @@ -93,7 +92,7 @@ static inline void fpstate_init_xstate(struct xregs_state *xsave)
> * XRSTORS requires these bits set in xcomp_bv, or it will
> * trigger #GP:
> */
> - xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask_user;
> + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask_all;
> }
>
> static inline void fpstate_init_fxstate(struct fxregs_state *fx)
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 76f83d2ac10e..d8e2ec99f635 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -19,9 +19,6 @@
> #define XSAVE_YMM_SIZE 256
> #define XSAVE_YMM_OFFSET (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>
> -/* Supervisor features */
> -#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> -
> /* All currently supported features */
> #define SUPPORTED_XFEATURES_MASK (XFEATURE_MASK_FP | \
> XFEATURE_MASK_SSE | \
> @@ -40,6 +37,7 @@
> #endif
>
> extern u64 xfeatures_mask_user;
> +extern u64 xfeatures_mask_all;
You have a bunch of places where you generate the system mask by doing
~xfeatures_mask_user.
Why not define
xfeatures_mask_system
instead and generate the _all mask at the places you need it by doing
xfeatures_mask_user | xfeatures_mask_system
?
We are differentiating user and system states now so it is only logical
to have that mirrored in the variables, right?
You even do that in fpu__init_system_xstate().
...
> @@ -225,20 +230,19 @@ void fpu__init_cpu_xstate(void)
> * set here.
> */
>
> - xfeatures_mask_user &= ~XFEATURE_MASK_SUPERVISOR;
> -
> cr4_set_bits(X86_CR4_OSXSAVE);
> xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user);
<---- newline here.
> + /*
> + * MSR_IA32_XSS sets which XSAVES system states to be managed by
Improve:
"MSR_IA32_XSS controls which system (not user) states are going to be
managed by XSAVES."
> @@ -702,6 +703,7 @@ static int init_xstate_size(void)
> */
> static void fpu__init_disable_system_xstate(void)
> {
> + xfeatures_mask_all = 0;
> xfeatures_mask_user = 0;
> cr4_clear_bits(X86_CR4_OSXSAVE);
> fpu__xstate_clear_all_cpu_caps();
> @@ -717,6 +719,8 @@ void __init fpu__init_system_xstate(void)
> static int on_boot_cpu __initdata = 1;
> int err;
> int i;
> + u64 cpu_user_xfeatures_mask;
> + u64 cpu_system_xfeatures_mask;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
>
> WARN_ON_FPU(!on_boot_cpu);
> on_boot_cpu = 0;
...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH v3 2/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-17 9:57 UTC (permalink / raw)
To: Al Viro
Cc: keescook, linux-kernel, ebiederm, mcgrof, akpm, joe.lawrence,
longman, linux, adobriyan, linux-api, Andrea Arcangeli,
Miklos Szeredi, Eric Dumazet
In-Reply-To: <20181017003548.GA32577@ZenIV.linux.org.uk>
On Wed, Oct 17, 2018 at 01:35:48AM +0100, Al Viro wrote:
> On Wed, Oct 17, 2018 at 12:33:22AM +0200, Christian Brauner wrote:
> > Currently, when writing
> >
> > echo 18446744073709551616 > /proc/sys/fs/file-max
> >
> > /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> > crashes the system.
> > This commit sets the max and min value for file-max and returns -EINVAL
> > when a long int is exceeded. Any higher value cannot currently be used as
> > the percpu counters are long ints and not unsigned integers. This behavior
> > also aligns with other tuneables that return -EINVAL when their range is
> > exceeded. See e.g. [1], [2] and others.
>
> Mostly sane, but... get_max_files() users are bloody odd. The one in
> file-max limit reporting looks like a half-arsed attempt in "[PATCH] fix
> file counting". The one in af_unix.c, though... I don't remember how
> that check had come to be - IIRC that was a strange fallout of a thread
> with me, Andrea and ANK involved, circa 1999, but I don't remember details;
> Andrea, any memories? It might be worth reconsidering... The change in
> question is in 2.2.4pre6; what do we use unix_nr_socks for? We try to
> limit the number of PF_UNIX socks by 2 * max_files, but max_files can be
So that's something I mentioned to Kees before. It seems we should
either simply replace this check with:
if ((atomic_long_read(&unix_nr_socks) >> 1) > get_max_files())
goto out;
to protect against overflows or simply do
if (atomic_long_read(&unix_nr_socks) > get_max_files())
goto out;
> huge *and* non-constant (i.e. it can decrease). What's more, unix_tot_inflight
> is unsigned int and max_files might exceed 2^31 just fine since "fs: allow
> for more than 2^31 files" back in 2010... Something's fishy there...
What's more is that fs/file_table.c:files_maxfiles_init()
currently has:
void __init files_maxfiles_init(void)
{
unsigned long n;
unsigned long memreserve = (totalram_pages - nr_free_pages()) * 3/2;
memreserve = min(memreserve, totalram_pages - 1);
n = ((totalram_pages - memreserve) * (PAGE_SIZE / 1024)) / 10;
files_stat.max_files = max_t(unsigned long, n, NR_FILE);
}
given that we currently can't handle more than LONG_MAX files should we
maybe cap here? Like:
diff --git a/fs/file_table.c b/fs/file_table.c
index e49af4caf15d..dd108b4c6d72 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -376,6 +376,8 @@ void __init files_init(void)
/*
* One file with associated inode and dcache is very roughly 1K. Per default
* do not use more than 10% of our memory for files.
+ * The percpu counters only handle long ints so cap maximum number of
+ * files at LONG_MAX.
*/
void __init files_maxfiles_init(void)
{
@@ -386,4 +388,7 @@ void __init files_maxfiles_init(void)
n = ((totalram_pages - memreserve) * (PAGE_SIZE / 1024)) / 10;
files_stat.max_files = max_t(unsigned long, n, NR_FILE);
+
+ if (files_stat.max_files > LONG_MAX)
+ files_stat.max_files = LONG_MAX;
}
^ permalink raw reply related
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Srikar Dronamraju @ 2018-10-17 7:19 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E . McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas,
Will Deacon
In-Reply-To: <20181010191936.7495-7-mathieu.desnoyers@efficios.com>
Hi Mathieu,
> +static int do_cpu_opv(struct cpu_op *cpuop, int cpuopcnt,
> + struct cpu_opv_vaddr *vaddr_ptrs, int cpu)
> +{
> + struct mm_struct *mm = current->mm;
> + int ret;
> +
> +retry:
> + if (cpu != raw_smp_processor_id()) {
> + ret = push_task_to_cpu(current, cpu);
> + if (ret)
> + goto check_online;
> + }
> + down_read(&mm->mmap_sem);
> + ret = vaddr_ptrs_check(vaddr_ptrs);
> + if (ret)
> + goto end;
> + preempt_disable();
> + if (cpu != smp_processor_id()) {
> + preempt_enable();
> + up_read(&mm->mmap_sem);
> + goto retry;
> + }
If we have a higher priority task/s either pinned to the cpu, dont we end up
in busy-looping till the task exits/sleeps?
> + ret = __do_cpu_opv(cpuop, cpuopcnt);
> + preempt_enable();
> +end:
> + up_read(&mm->mmap_sem);
> + return ret;
> +
> +check_online:
> + /*
> + * push_task_to_cpu() returns -EINVAL if the requested cpu is not part
> + * of the current thread's cpus_allowed mask.
> + */
> + if (ret == -EINVAL)
> + return ret;
> + get_online_cpus();
> + if (cpu_online(cpu)) {
> + put_online_cpus();
> + goto retry;
> + }
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-17 7:05 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810161416540.83080@chino.kir.corp.google.com>
On Tue 16-10-18 14:24:19, David Rientjes wrote:
> On Tue, 16 Oct 2018, Michal Hocko wrote:
>
> > > I don't understand the point of extending smaps with yet another line.
> >
> > Because abusing a vma flag part is just wrong. What are you going to do
> > when a next bug report states that the flag is set even though no
> > userspace has set it and that leads to some malfunctioning? Can you rule
> > that out? Even your abuse of the flag is surprising so why others
> > wouldn't be?
> >
>
> The flag has taken on the meaning of "thp disabled for this vma", how it
> is set is not the scope of the flag. If a thp is explicitly disabled from
> being eligible for thp, whether by madvise, prctl, or any future
> mechanism, it should use VM_NOHUGEPAGE or show_smap_vma_flags() needs to
> be modified.
No, this is not the meaning which is documented
nh - no-huge page advise flag
and as far as I know it is only you who has complained so far.
> > As I've said there are two things. Exporting PR_SET_THP_DISABLE to
> > userspace so that a 3rd party process can query it. I've already
> > explained why that might be useful. If you really insist on having
> > a per-vma field then let's do it properly now. Are you going to agree on
> > that? If yes, I am willing to spend my time on that but I am not going
> > to bother if this will lead to "I want my vma field abuse anyway".
>
> I think what you and I want is largely irrelevant :) What's important is
> that there are userspace implementations that query this today so
> continuing to support it as the way to determine if a vma has been thp
> disabled doesn't seem problematic and guarantees that userspace doesn't
> break.
Do you know of any other userspace except your usecase? Is there
anything fundamental that would prevent a proper API adoption for you?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH for 4.21 03/16] sched: Implement push_task_to_cpu (v2)
From: Srikar Dronamraju @ 2018-10-17 6:51 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E . McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas,
Will Deacon
In-Reply-To: <20181010191936.7495-4-mathieu.desnoyers@efficios.com>
Hi Mathieu,
> +int push_task_to_cpu(struct task_struct *p, unsigned int dest_cpu)
> +{
In your use case, is the task going to be current?
If yes, we should simply be using migrate_task_to.
> + struct rq_flags rf;
> + struct rq *rq;
> + int ret = 0;
> +
> + rq = task_rq_lock(p, &rf);
> + update_rq_clock(rq);
> +
> + if (!cpumask_test_cpu(dest_cpu, &p->cpus_allowed)) {
> + ret = -EINVAL;
> + goto out;
> + }
Ideally we should have checked cpus_allowed/cpu_active_mask before taking
the lock. This would help reduce the contention on the rqlock when the
passed parameter is not correct.
> +
> + if (!cpumask_test_cpu(dest_cpu, cpu_active_mask)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + if (task_cpu(p) == dest_cpu)
> + goto out;
Same as above.
> +
> + if (task_running(rq, p) || p->state == TASK_WAKING) {
Why are we using migration thread to move a task in TASK_WAKING state?
> + struct migration_arg arg = { p, dest_cpu };
> + /* Need help from migration thread: drop lock and wait. */
> + task_rq_unlock(rq, p, &rf);
> + stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
> + tlb_migrate_finish(p->mm);
> + return 0;
Why cant we use migrate_task_to instead?
> + } else if (task_on_rq_queued(p)) {
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 455fa330de04..27ad25780204 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1340,6 +1340,15 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
> #endif
> }
>
> +#ifdef CONFIG_SMP
> +int push_task_to_cpu(struct task_struct *p, unsigned int dest_cpu);
> +#else
> +static inline int push_task_to_cpu(struct task_struct *p, unsigned int dest_cpu)
> +{
> + return 0;
> +}
> +#endif
> +
Your usecase is outside kernel/sched. So I am not sure if this is the right
place for the declaration.
^ permalink raw reply
* Re: [PATCH v3 1/2] sysctl: handle overflow in proc_get_long
From: Kees Cook @ 2018-10-17 2:19 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W . Biederman, LKML, Luis R. Rodriguez, Andrew Morton,
Joe Lawrence, Waiman Long, Dominik Brodowski, Al Viro,
Alexey Dobriyan, Linux API
In-Reply-To: <CAHrFyr6AeZzfGkQneiyoLz6YZ-vnyyQWTtc0HK-tqVJkoXt=4w@mail.gmail.com>
On Tue, Oct 16, 2018 at 5:24 PM, Christian Brauner <christian@brauner.io> wrote:
> Right, but if you write a value that exceeds the buffer of 22 chars that is used
> to parse you already get EINVAL back on current kernels.
> So it didn't feel like returning EOVERFLOW or ERANGE might make sense.
> I saw a change in 4.10 or 4.11 as well that used EINVAL on UINT_MAX overflow
> or something. EINVAL might be enough information for userspace here ?/.
I'd agree: I think there is more precedent for EINVAL.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Sergey Senozhatsky @ 2018-10-17 1:46 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Sergey Senozhatsky, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
linux-kernel, linux-api, Thomas Gleixner, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Linus Torvalds
In-Reply-To: <1984292897.263.1539717457933.JavaMail.zimbra@efficios.com>
Hi Mathieu,
On (10/16/18 15:17), Mathieu Desnoyers wrote:
>
> Therefore, only an internal kernel bug between vm_map_user_ram() and
> vm_unmap_user_ram() should trigger the BUG_ON(). No user input is passed
> to vm_unmap_user_ram().
>
> Now, let's look at vm_map_user_ram(). It calls alloc_vmap_area(), which returns
> a vmap_area. Then if vmap_page_range failed, vm_unmap_user_ram is called on the
> memory that has just been returned by vm_map_user_ram. Again, only an internal
> bug between map/unmap can trigger the BUG_ON() in vm_unmap_user_ram.
Thanks for spending time on this.
Just wanted someone to have extra look at syscall->BUG_ON().
-ss
^ permalink raw reply
* Re: [PATCH v3 2/2] sysctl: handle overflow for file-max
From: Al Viro @ 2018-10-17 0:35 UTC (permalink / raw)
To: Christian Brauner
Cc: keescook, linux-kernel, ebiederm, mcgrof, akpm, joe.lawrence,
longman, linux, adobriyan, linux-api, Andrea Arcangeli,
Miklos Szeredi, Eric Dumazet
In-Reply-To: <20181016223322.16844-3-christian@brauner.io>
On Wed, Oct 17, 2018 at 12:33:22AM +0200, Christian Brauner wrote:
> Currently, when writing
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
>
> /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> crashes the system.
> This commit sets the max and min value for file-max and returns -EINVAL
> when a long int is exceeded. Any higher value cannot currently be used as
> the percpu counters are long ints and not unsigned integers. This behavior
> also aligns with other tuneables that return -EINVAL when their range is
> exceeded. See e.g. [1], [2] and others.
Mostly sane, but... get_max_files() users are bloody odd. The one in
file-max limit reporting looks like a half-arsed attempt in "[PATCH] fix
file counting". The one in af_unix.c, though... I don't remember how
that check had come to be - IIRC that was a strange fallout of a thread
with me, Andrea and ANK involved, circa 1999, but I don't remember details;
Andrea, any memories? It might be worth reconsidering... The change in
question is in 2.2.4pre6; what do we use unix_nr_socks for? We try to
limit the number of PF_UNIX socks by 2 * max_files, but max_files can be
huge *and* non-constant (i.e. it can decrease). What's more, unix_tot_inflight
is unsigned int and max_files might exceed 2^31 just fine since "fs: allow
for more than 2^31 files" back in 2010... Something's fishy there...
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Sergey Senozhatsky @ 2018-10-17 0:27 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, Peter Zijlstra, Paul E . McKenney, Boqun Feng,
linux-kernel, linux-api, Thomas Gleixner, Andy Lutomirski,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H . Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <20181016143016.10da89bd@gandalf.local.home>
On (10/16/18 14:30), Steven Rostedt wrote:
> > +void vm_unmap_user_ram(const void *mem, unsigned int count)
> > +{
> > + unsigned long size = (unsigned long)count << PAGE_SHIFT;
> > + unsigned long addr = (unsigned long)mem;
> > + struct vmap_area *va;
> > +
> > + might_sleep();
> > + BUG_ON(!addr);
> > + BUG_ON(addr < VMALLOC_START);
> > + BUG_ON(addr > VMALLOC_END);
> > + BUG_ON(!PAGE_ALIGNED(addr));
> > +
> > + debug_check_no_locks_freed(mem, size);
> > + va = find_vmap_area(addr);
> > + BUG_ON(!va);
> > + free_unmap_vmap_area(va);
> > +}
> > +EXPORT_SYMBOL(vm_unmap_user_ram);
> > +
>
> Noticing this from Sergey's question in another patch, why are you
> using BUG_ON()? That's rather extreme and something we are trying to
> avoid adding more of (I still need to remove the BUG_ON()s I've added
> over ten years ago). I don't see why all these BUG_ON's can't be turned
> into:
+1
> if (WARN_ON(x))
> return;
Given that this somewhat MM-related, I'd may be say VM_WARN_ON().
-ss
^ permalink raw reply
* Re: [PATCH v3 1/2] sysctl: handle overflow in proc_get_long
From: Christian Brauner @ 2018-10-17 0:24 UTC (permalink / raw)
To: Eric W . Biederman
Cc: Kees Cook, LKML, Luis R. Rodriguez, Andrew Morton, Joe Lawrence,
Waiman Long, Dominik Brodowski, Al Viro, Alexey Dobriyan,
Linux API
In-Reply-To: <877eihjw0n.fsf@xmission.com>
On Wed, Oct 17, 2018 at 1:46 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Christian Brauner <christian@brauner.io> writes:
>
> > proc_get_long() is a funny function. It uses simple_strtoul() and for a
> > good reason. proc_get_long() wants to always succeed the parse and return
> > the maybe incorrect value and the trailing characters to check against a
> > pre-defined list of acceptable trailing values.
> > However, simple_strtoul() explicitly ignores overflows which can cause
> > funny things like the following to happen:
> >
> > echo 18446744073709551616 > /proc/sys/fs/file-max
> > cat /proc/sys/fs/file-max
> > 0
> >
> > (Which will cause your system to silently die behind your back.)
> >
> > On the other hand kstrtoul() does do overflow detection but does not return
> > the trailing characters, and also fails the parse when anything other than
> > '\n' is a trailing character whereas proc_get_long() wants to be more
> > lenient.
> >
> > Now, before adding another kstrtoul() function let's simply add a static
> > parse strtoul_lenient() which:
> > - fails on overflow with -ERANGE
> > - returns the trailing characters to the caller
> >
> > The reason why we should fail on ERANGE is that we already do a partial
> > fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we
> > already reject values such as 184467440737095516160 (21 chars) but accept
> > values such as 18446744073709551616 (20 chars) but both are overflows. So
> > we should just always reject 64bit overflows and not special-case this
> > based on the number of chars.
> >
> > Acked-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > v2->v3:
> > - (Kees) s/#include <../lib/kstrtox.h>/#include "../lib/kstrtox.h"/g
> > - (Kees) document strtoul_lenient()
> >
> > v1->v2:
> > - s/sysctl_cap_erange/sysctl_lenient/g
> > - consistenly fail on overflow
> >
> > v0->v1:
> > - s/sysctl_strtoul_lenient/strtoul_cap_erange/g
> > - (Al) remove bool overflow return argument from strtoul_cap_erange
> > - (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
> > - (Dominik) fix spelling in commit message
> > ---
> > kernel/sysctl.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index cc02050fd0c4..102aa7a65687 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -68,6 +68,8 @@
> > #include <linux/mount.h>
> > #include <linux/pipe_fs_i.h>
> >
> > +#include "../lib/kstrtox.h"
> > +
> > #include <linux/uaccess.h>
> > #include <asm/processor.h>
> >
> > @@ -2065,6 +2067,41 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
> > }
> > }
> >
> > +/**
> > + * strtoul_lenient - parse an ASCII formatted integer from a buffer and only
> > + * fail on overflow
> > + *
> > + * @cp: kernel buffer containing the string to parse
> > + * @endp: pointer to store the trailing characters
> > + * @base: the base to use
> > + * @res: where the parsed integer will be stored
> > + *
> > + * In case of success 0 is returned and @res will contain the parsed integer,
> > + * @endp will hold any trailing characters.
> > + * This function will fail the parse on overflow. If there wasn't an overflow
> > + * the function will defer the decision what characters count as invalid to the
> > + * caller.
> > + */
> > +static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
> > + unsigned long *res)
> > +{
> > + unsigned long long result;
> > + unsigned int rv;
> > +
> > + cp = _parse_integer_fixup_radix(cp, &base);
> > + rv = _parse_integer(cp, base, &result);
> > + if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
> > + return -ERANGE;
> > +
> > + cp += rv;
> > +
> > + if (endp)
> > + *endp = (char *)cp;
> > +
> > + *res = (unsigned long)result;
> > + return 0;
> > +}
> > +
> > #define TMPBUFLEN 22
> > /**
> > * proc_get_long - reads an ASCII formatted integer from a user buffer
> > @@ -2108,7 +2145,8 @@ static int proc_get_long(char **buf, size_t *size,
> > if (!isdigit(*p))
> > return -EINVAL;
> >
> > - *val = simple_strtoul(p, &p, 0);
> > + if (strtoul_lenient(p, &p, 0, val))
> > + return -EINVAL;
>
> Is it deliberate that on an error stroul_lenient returns -ERANGE but
> then proc_get_long returns -EINVAL? That feels wrong. The write system
Yes, because all other integer parsing function return ERANGE as well.
Right now there are no other users but if someone wants to use that function
I wanted it to behave like the others.
> call does not permit -ERANGE or -EINVAL for the contents of the data
> so both options appear equally bad from a standards point of view.
Right, but if you write a value that exceeds the buffer of 22 chars that is used
to parse you already get EINVAL back on current kernels.
So it didn't feel like returning EOVERFLOW or ERANGE might make sense.
I saw a change in 4.10 or 4.11 as well that used EINVAL on UINT_MAX overflow
or something. EINVAL might be enough information for userspace here ?/.
>
> I am just wondering what the thinking is here.
>
> > len = p - tmp;
^ permalink raw reply
* Re: [PATCH v3 1/2] sysctl: handle overflow in proc_get_long
From: Eric W. Biederman @ 2018-10-16 23:45 UTC (permalink / raw)
To: Christian Brauner
Cc: keescook, linux-kernel, mcgrof, akpm, joe.lawrence, longman,
linux, viro, adobriyan, linux-api
In-Reply-To: <20181016223322.16844-2-christian@brauner.io>
Christian Brauner <christian@brauner.io> writes:
> proc_get_long() is a funny function. It uses simple_strtoul() and for a
> good reason. proc_get_long() wants to always succeed the parse and return
> the maybe incorrect value and the trailing characters to check against a
> pre-defined list of acceptable trailing values.
> However, simple_strtoul() explicitly ignores overflows which can cause
> funny things like the following to happen:
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
> cat /proc/sys/fs/file-max
> 0
>
> (Which will cause your system to silently die behind your back.)
>
> On the other hand kstrtoul() does do overflow detection but does not return
> the trailing characters, and also fails the parse when anything other than
> '\n' is a trailing character whereas proc_get_long() wants to be more
> lenient.
>
> Now, before adding another kstrtoul() function let's simply add a static
> parse strtoul_lenient() which:
> - fails on overflow with -ERANGE
> - returns the trailing characters to the caller
>
> The reason why we should fail on ERANGE is that we already do a partial
> fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we
> already reject values such as 184467440737095516160 (21 chars) but accept
> values such as 18446744073709551616 (20 chars) but both are overflows. So
> we should just always reject 64bit overflows and not special-case this
> based on the number of chars.
>
> Acked-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> v2->v3:
> - (Kees) s/#include <../lib/kstrtox.h>/#include "../lib/kstrtox.h"/g
> - (Kees) document strtoul_lenient()
>
> v1->v2:
> - s/sysctl_cap_erange/sysctl_lenient/g
> - consistenly fail on overflow
>
> v0->v1:
> - s/sysctl_strtoul_lenient/strtoul_cap_erange/g
> - (Al) remove bool overflow return argument from strtoul_cap_erange
> - (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
> - (Dominik) fix spelling in commit message
> ---
> kernel/sysctl.c | 40 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index cc02050fd0c4..102aa7a65687 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -68,6 +68,8 @@
> #include <linux/mount.h>
> #include <linux/pipe_fs_i.h>
>
> +#include "../lib/kstrtox.h"
> +
> #include <linux/uaccess.h>
> #include <asm/processor.h>
>
> @@ -2065,6 +2067,41 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
> }
> }
>
> +/**
> + * strtoul_lenient - parse an ASCII formatted integer from a buffer and only
> + * fail on overflow
> + *
> + * @cp: kernel buffer containing the string to parse
> + * @endp: pointer to store the trailing characters
> + * @base: the base to use
> + * @res: where the parsed integer will be stored
> + *
> + * In case of success 0 is returned and @res will contain the parsed integer,
> + * @endp will hold any trailing characters.
> + * This function will fail the parse on overflow. If there wasn't an overflow
> + * the function will defer the decision what characters count as invalid to the
> + * caller.
> + */
> +static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
> + unsigned long *res)
> +{
> + unsigned long long result;
> + unsigned int rv;
> +
> + cp = _parse_integer_fixup_radix(cp, &base);
> + rv = _parse_integer(cp, base, &result);
> + if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
> + return -ERANGE;
> +
> + cp += rv;
> +
> + if (endp)
> + *endp = (char *)cp;
> +
> + *res = (unsigned long)result;
> + return 0;
> +}
> +
> #define TMPBUFLEN 22
> /**
> * proc_get_long - reads an ASCII formatted integer from a user buffer
> @@ -2108,7 +2145,8 @@ static int proc_get_long(char **buf, size_t *size,
> if (!isdigit(*p))
> return -EINVAL;
>
> - *val = simple_strtoul(p, &p, 0);
> + if (strtoul_lenient(p, &p, 0, val))
> + return -EINVAL;
Is it deliberate that on an error stroul_lenient returns -ERANGE but
then proc_get_long returns -EINVAL? That feels wrong. The write system
call does not permit -ERANGE or -EINVAL for the contents of the data
so both options appear equally bad from a standards point of view.
I am just wondering what the thinking is here.
> len = p - tmp;
^ permalink raw reply
* Re: [PATCH v3 0/2] sysctl: handle overflow for file-max
From: Kees Cook @ 2018-10-16 22:36 UTC (permalink / raw)
To: Christian Brauner, Andrew Morton
Cc: LKML, Eric W. Biederman, Luis R. Rodriguez, Joe Lawrence,
Waiman Long, Dominik Brodowski, Al Viro, Alexey Dobriyan,
Linux API
In-Reply-To: <20181016223322.16844-1-christian@brauner.io>
On Tue, Oct 16, 2018 at 3:33 PM, Christian Brauner <christian@brauner.io> wrote:
> Hey,
>
> Here is v3 of this patchset. Changelogs are in the individual commits.
Thanks! These look good. Andrew, can you take these?
-Kees
>
> Currently, when writing
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
>
> /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> crashes the system.
>
> The first version of this patch intended to detect the overflow and cap
> at ULONG_MAX. However, we should not do this and rather return EINVAL on
> overflow. The reasons are:
> - this aligns with other sysctl handlers that simply reject overflows
> (cf. [1], [2], and a bunch of others)
> - we already do a partial fail on overflow right now
> Namely, when the TMPBUFLEN is exceeded. So we already reject values
> such as 184467440737095516160 (21 chars) but accept values such as
> 18446744073709551616 (20 chars) but both are overflows. So we should
> just always reject 64bit overflows and not special-case this based on
> the number of chars.
>
> (This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
>
> Thanks!
> Christian
>
> [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
> [2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
>
> Christian Brauner (2):
> sysctl: handle overflow in proc_get_long
> sysctl: handle overflow for file-max
>
> kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
>
> --
> 2.17.1
>
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH v3 2/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-16 22:33 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
In-Reply-To: <20181016223322.16844-1-christian@brauner.io>
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
This commit sets the max and min value for file-max and returns -EINVAL
when a long int is exceeded. Any higher value cannot currently be used as
the percpu counters are long ints and not unsigned integers. This behavior
also aligns with other tuneables that return -EINVAL when their range is
exceeded. See e.g. [1], [2] and others.
[1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
[2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v2->v3:
- unchanged
v2->v1:
- consistenly fail on overflow
v0->v1:
- if max value is < than ULONG_MAX use max as upper bound
- (Dominik) remove double "the" from commit message
---
kernel/sysctl.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 102aa7a65687..93456e3a90cd 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -128,6 +128,7 @@ static int __maybe_unused one = 1;
static int __maybe_unused two = 2;
static int __maybe_unused four = 4;
static unsigned long one_ul = 1;
+static unsigned long long_max = LONG_MAX;
static int one_hundred = 100;
static int one_thousand = 1000;
#ifdef CONFIG_PRINTK
@@ -1697,6 +1698,8 @@ static struct ctl_table fs_table[] = {
.maxlen = sizeof(files_stat.max_files),
.mode = 0644,
.proc_handler = proc_doulongvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &long_max,
},
{
.procname = "nr_open",
@@ -2813,6 +2816,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
break;
if (neg)
continue;
+ if ((max && val > *max) || (min && val < *min)) {
+ err = -EINVAL;
+ break;
+ }
val = convmul * val / convdiv;
if ((min && val < *min) || (max && val > *max))
continue;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/2] sysctl: handle overflow in proc_get_long
From: Christian Brauner @ 2018-10-16 22:33 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
In-Reply-To: <20181016223322.16844-1-christian@brauner.io>
proc_get_long() is a funny function. It uses simple_strtoul() and for a
good reason. proc_get_long() wants to always succeed the parse and return
the maybe incorrect value and the trailing characters to check against a
pre-defined list of acceptable trailing values.
However, simple_strtoul() explicitly ignores overflows which can cause
funny things like the following to happen:
echo 18446744073709551616 > /proc/sys/fs/file-max
cat /proc/sys/fs/file-max
0
(Which will cause your system to silently die behind your back.)
On the other hand kstrtoul() does do overflow detection but does not return
the trailing characters, and also fails the parse when anything other than
'\n' is a trailing character whereas proc_get_long() wants to be more
lenient.
Now, before adding another kstrtoul() function let's simply add a static
parse strtoul_lenient() which:
- fails on overflow with -ERANGE
- returns the trailing characters to the caller
The reason why we should fail on ERANGE is that we already do a partial
fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we
already reject values such as 184467440737095516160 (21 chars) but accept
values such as 18446744073709551616 (20 chars) but both are overflows. So
we should just always reject 64bit overflows and not special-case this
based on the number of chars.
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
v2->v3:
- (Kees) s/#include <../lib/kstrtox.h>/#include "../lib/kstrtox.h"/g
- (Kees) document strtoul_lenient()
v1->v2:
- s/sysctl_cap_erange/sysctl_lenient/g
- consistenly fail on overflow
v0->v1:
- s/sysctl_strtoul_lenient/strtoul_cap_erange/g
- (Al) remove bool overflow return argument from strtoul_cap_erange
- (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
- (Dominik) fix spelling in commit message
---
kernel/sysctl.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cc02050fd0c4..102aa7a65687 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -68,6 +68,8 @@
#include <linux/mount.h>
#include <linux/pipe_fs_i.h>
+#include "../lib/kstrtox.h"
+
#include <linux/uaccess.h>
#include <asm/processor.h>
@@ -2065,6 +2067,41 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
}
}
+/**
+ * strtoul_lenient - parse an ASCII formatted integer from a buffer and only
+ * fail on overflow
+ *
+ * @cp: kernel buffer containing the string to parse
+ * @endp: pointer to store the trailing characters
+ * @base: the base to use
+ * @res: where the parsed integer will be stored
+ *
+ * In case of success 0 is returned and @res will contain the parsed integer,
+ * @endp will hold any trailing characters.
+ * This function will fail the parse on overflow. If there wasn't an overflow
+ * the function will defer the decision what characters count as invalid to the
+ * caller.
+ */
+static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
+ unsigned long *res)
+{
+ unsigned long long result;
+ unsigned int rv;
+
+ cp = _parse_integer_fixup_radix(cp, &base);
+ rv = _parse_integer(cp, base, &result);
+ if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
+ return -ERANGE;
+
+ cp += rv;
+
+ if (endp)
+ *endp = (char *)cp;
+
+ *res = (unsigned long)result;
+ return 0;
+}
+
#define TMPBUFLEN 22
/**
* proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2108,7 +2145,8 @@ static int proc_get_long(char **buf, size_t *size,
if (!isdigit(*p))
return -EINVAL;
- *val = simple_strtoul(p, &p, 0);
+ if (strtoul_lenient(p, &p, 0, val))
+ return -EINVAL;
len = p - tmp;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-16 22:33 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
Hey,
Here is v3 of this patchset. Changelogs are in the individual commits.
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
The first version of this patch intended to detect the overflow and cap
at ULONG_MAX. However, we should not do this and rather return EINVAL on
overflow. The reasons are:
- this aligns with other sysctl handlers that simply reject overflows
(cf. [1], [2], and a bunch of others)
- we already do a partial fail on overflow right now
Namely, when the TMPBUFLEN is exceeded. So we already reject values
such as 184467440737095516160 (21 chars) but accept values such as
18446744073709551616 (20 chars) but both are overflows. So we should
just always reject 64bit overflows and not special-case this based on
the number of chars.
(This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
Thanks!
Christian
[1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
[2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
Christian Brauner (2):
sysctl: handle overflow in proc_get_long
sysctl: handle overflow for file-max
kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply
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