* Re: [PATCHv4 RESEND 0/3] syscalls,x86: Add execveat() system call
From: Eric W. Biederman @ 2014-10-21 4:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Drysdale, Alexander Viro, Meredydd Luff,
linux-kernel@vger.kernel.org, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Andrew Morton, Kees Cook, Arnd Bergmann, X86 ML,
linux-arch, Linux API
In-Reply-To: <CALCETrXBjLZTWVJfcsE4NA-JP9zSSgn=6ND0=cZ9BTy=CoN7pA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
> On Mon, Oct 20, 2014 at 6:48 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Sun, Oct 19, 2014 at 1:20 AM, Eric W. Biederman
>> <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>>>
>>>> [Added Eric Biederman, since I think your tree might be a reasonable
>>>> route forward for these patches.]
>>>>
>>>> On Thu, Jun 5, 2014 at 6:40 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> Resending, adding cc:linux-api.
>>>>>
>>>>> Also, it may help to add a little more background -- this patch is
>>>>> needed as a (small) part of implementing Capsicum in the Linux kernel.
>>>>>
>>>>> Capsicum is a security framework that has been present in FreeBSD since
>>>>> version 9.0 (Jan 2012), and is based on concepts from object-capability
>>>>> security [1].
>>>>>
>>>>> One of the features of Capsicum is capability mode, which locks down
>>>>> access to global namespaces such as the filesystem hierarchy. In
>>>>> capability mode, /proc is thus inaccessible and so fexecve(3) doesn't
>>>>> work -- hence the need for a kernel-space
>>>>
>>>> I just found myself wanting this syscall for another reason: injecting
>>>> programs into sandboxes or otherwise heavily locked-down namespaces.
>>>>
>>>> For example, I want to be able to reliably do something like nsenter
>>>> --namespace-flags-here toybox sh. Toybox's shell is unusual in that
>>>> it is more or less fully functional, so this should Just Work (tm),
>>>> except that the toybox binary might not exist in the namespace being
>>>> entered. If execveat were available, I could rig nsenter or a similar
>>>> tool to open it with O_CLOEXEC, enter the namespace, and then call
>>>> execveat.
>>>>
>>>> Is there any reason that these patches can't be merged more or less as
>>>> is for 3.19?
>>>
>>> Yes. There is a silliness in how it implements fexecve. The fexecve
>>> case should be use the empty string "" not a NULL pointer to indication
>>> that. That change will then harmonize execveat with the other ...at
>>> system calls and simplify the code and remove a special case. I believe
>>> using the empty string "" requires implementing the AT_EMPTY_PATH flag.
>>
>> Good point -- I'll shift to "" + AT_EMPTY_PATH.
>
> Pending a better idea, I would also see if the patches can be changed
> to return an error if d_path ends up with an "(unreachable)" thing
> rather than failing inexplicably later on.
For my reference we are talking about
> @@ -1489,7 +1524,21 @@ static int do_execve_common(struct filename *filename,
> sched_exec();
>
> bprm->file = file;
> - bprm->filename = bprm->interp = filename->name;
> + if (filename && fd == AT_FDCWD) {
> + bprm->filename = filename->name;
> + } else {
> + pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
> + if (!pathbuf) {
> + retval = -ENOMEM;
> + goto out_unmark;
> + }
> + bprm->filename = d_path(&file->f_path, pathbuf, PATH_MAX);
> + if (IS_ERR(bprm->filename)) {
> + retval = PTR_ERR(bprm->filename);
> + goto out_unmark;
> + }
> + }
> + bprm->interp = bprm->filename;
>
> retval = bprm_mm_init(bprm);
> if (retval)
The interesting case for fexecve is when we either don't know what files
are present or we don't want to depend on which files are present.
As Al pointed out d_path really isn't the right solution. It fails when
printing /proc/self/fd/${fd}/${filename->name} would work, and the
"(deleted)" or "(unreachable)" strings are wrong.
The test for today's cases should be:
if ((filename->name[0] == '/') || fd == AT_FDCWD) {
bprm->filename = filename->name;
}
To handle the case where the file descriptor is relevant.
For the case where the file descriptor is relevant let me suggest
setting bprm->filename and bprm->interp to:
/dev/fd/${fd}/${filename->name}
It is more a description of what we have done but as a magic string it
is descriptive. Documetation/devices.txt documents that /dev/fd/ should
exist, making it an unambiguous path. Further these days the kernel
sets the device naming policy in dev, so I think we are strongly safe in
using that path in any event.
I think execveat is interesting in the kernel because the motivating
cases are the cases where anything except a static executable is
uninteresting.
Now it has been suggested creating a dupfs or a mini-proc. I think that
sounds like a nice companion, to the concept of a locked down root.
But I don't think it removes the need for execveat (because we still
have the case where we don't want to care what is mounted, and are happy
to use static executables).
Eric
^ permalink raw reply
* Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
From: Eric W. Biederman @ 2014-10-21 4:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Serge E. Hallyn, Aditya Kali, Linux API, Linux Containers,
Serge Hallyn, linux-kernel@vger.kernel.org, Tejun Heo,
cgroups-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar
In-Reply-To: <CALCETrXhGnBM_xx=Auz3WRQXkqhGGTWuZN=PU+A9HZ7Ek27FLA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
> On Sun, Oct 19, 2014 at 9:55 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>
>>
>> On October 19, 2014 1:26:29 PM CDT, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> Is the idea
>>>that you want a privileged user wrt a cgroupns's userns to be able to
>>>use this? If so:
>>>
>>>Yes, that current_cred() thing is bogus. (Actually, this is probably
>>>exploitable right now if any cgroup.procs inode anywhere on the system
>>>lets non-root write.) (Can we have some kernel debugging option that
>>>makes any use of current_cred() in write(2) warn?)
>>>
>>>We really need a weaker version of may_ptrace for this kind of stuff.
>>>Maybe the existing may_ptrace stuff is okay, actually. But this is
>>>completely missing group checks, cap checks, capabilities wrt the
>>>userns, etc.
>>>
>>>Also, I think that, if this version of the patchset allows non-init
>>>userns to unshare cgroupns, then the issue of what permission is
>>>needed to lock the cgroup hierarchy like that needs to be addressed,
>>>because unshare(CLONE_NEWUSER|CLONE_NEWCGROUP) will effectively pin
>>>the calling task with no permission required. Bolting on a fix later
>>>will be a mess.
>>
>> I imagine the pinning would be like the userns.
>>
>> Ah but there is a potentially serious issue with the pinning.
>> With pinning we can make it impossible for root to move us to a different cgroup.
>>
>> I am not certain how serious that is but it bears thinking about.
>> If we don't implement pinning we should be able to implent everything with just filesystem mount options, and no new namespace required.
>>
>> Sigh.
>>
>> I am too tired tonight to see the end game in this.
>
> Possible solution:
>
> Ditch the pinning. That is, if you're outside a cgroupns (or you have
> a non-ns-confined cgroupfs mounted), then you can move a task in a
> cgroupns outside of its root cgroup. If you do this, then the task
> thinks its cgroup is something like "../foo" or "../../foo".
Of the possible solutions that seems attractive to me, simply because
we sometimes want to allow clever things to occur.
Does anyone know of a reason (beyond pretty printing) why we need
cgroupns to restrict the subset of cgroups processes can be in?
I would expect permissions on the cgroup directories themselves, and
limited visiblilty would be (in general) to achieve the desired
visiblity.
> While we're at it, consider making setns for a cgroupns *not* change
> the caller's cgroup. Is there any reason it really needs to?
setns doesn't but nsenter is going to need to change the cgroup
if the pinning requirement is kept. nsenenter is going to want to
change the cgroup if the pinning requirement is dropped.
Eric
^ permalink raw reply
* Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
From: Andy Lutomirski @ 2014-10-21 5:03 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux API, Linux Containers, Serge Hallyn,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ingo Molnar,
Tejun Heo, cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87zjcq10ya.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Mon, Oct 20, 2014 at 9:49 PM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>
>> On Sun, Oct 19, 2014 at 9:55 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>>
>>>
>>> On October 19, 2014 1:26:29 PM CDT, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>
>>>> Is the idea
>>>>that you want a privileged user wrt a cgroupns's userns to be able to
>>>>use this? If so:
>>>>
>>>>Yes, that current_cred() thing is bogus. (Actually, this is probably
>>>>exploitable right now if any cgroup.procs inode anywhere on the system
>>>>lets non-root write.) (Can we have some kernel debugging option that
>>>>makes any use of current_cred() in write(2) warn?)
>>>>
>>>>We really need a weaker version of may_ptrace for this kind of stuff.
>>>>Maybe the existing may_ptrace stuff is okay, actually. But this is
>>>>completely missing group checks, cap checks, capabilities wrt the
>>>>userns, etc.
>>>>
>>>>Also, I think that, if this version of the patchset allows non-init
>>>>userns to unshare cgroupns, then the issue of what permission is
>>>>needed to lock the cgroup hierarchy like that needs to be addressed,
>>>>because unshare(CLONE_NEWUSER|CLONE_NEWCGROUP) will effectively pin
>>>>the calling task with no permission required. Bolting on a fix later
>>>>will be a mess.
>>>
>>> I imagine the pinning would be like the userns.
>>>
>>> Ah but there is a potentially serious issue with the pinning.
>>> With pinning we can make it impossible for root to move us to a different cgroup.
>>>
>>> I am not certain how serious that is but it bears thinking about.
>>> If we don't implement pinning we should be able to implent everything with just filesystem mount options, and no new namespace required.
>>>
>>> Sigh.
>>>
>>> I am too tired tonight to see the end game in this.
>>
>> Possible solution:
>>
>> Ditch the pinning. That is, if you're outside a cgroupns (or you have
>> a non-ns-confined cgroupfs mounted), then you can move a task in a
>> cgroupns outside of its root cgroup. If you do this, then the task
>> thinks its cgroup is something like "../foo" or "../../foo".
>
> Of the possible solutions that seems attractive to me, simply because
> we sometimes want to allow clever things to occur.
>
> Does anyone know of a reason (beyond pretty printing) why we need
> cgroupns to restrict the subset of cgroups processes can be in?
>
> I would expect permissions on the cgroup directories themselves, and
> limited visiblilty would be (in general) to achieve the desired
> visiblity.
This makes the security impact of cgroupns very easy to understand,
right? Because there really won't be any -- cgroupns only affects
reads from /proc and what cgroupfs shows, but it doesn't change any
actual cgroups, nor does it affect any cgroup *changes*.
>
>> While we're at it, consider making setns for a cgroupns *not* change
>> the caller's cgroup. Is there any reason it really needs to?
>
> setns doesn't but nsenter is going to need to change the cgroup
> if the pinning requirement is kept. nsenenter is going to want to
> change the cgroup if the pinning requirement is dropped.
>
It seems easy enough for nsenter to change the cgroup all by itself.
--Andy
^ permalink raw reply
* Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
From: Eric W. Biederman @ 2014-10-21 5:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Serge E. Hallyn, Aditya Kali, Linux API, Linux Containers,
Serge Hallyn, linux-kernel@vger.kernel.org, Tejun Heo,
cgroups-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar
In-Reply-To: <CALCETrVkMtsnEh57jFZrdx5vHbz97BdO7OuupT+xVNnWpJjxng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
> On Mon, Oct 20, 2014 at 9:49 PM, Eric W. Biederman
> <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>>
>>> On Sun, Oct 19, 2014 at 9:55 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>>>
>>>>
>>>> On October 19, 2014 1:26:29 PM CDT, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>
>>>>> Is the idea
>>>>>that you want a privileged user wrt a cgroupns's userns to be able to
>>>>>use this? If so:
>>>>>
>>>>>Yes, that current_cred() thing is bogus. (Actually, this is probably
>>>>>exploitable right now if any cgroup.procs inode anywhere on the system
>>>>>lets non-root write.) (Can we have some kernel debugging option that
>>>>>makes any use of current_cred() in write(2) warn?)
>>>>>
>>>>>We really need a weaker version of may_ptrace for this kind of stuff.
>>>>>Maybe the existing may_ptrace stuff is okay, actually. But this is
>>>>>completely missing group checks, cap checks, capabilities wrt the
>>>>>userns, etc.
>>>>>
>>>>>Also, I think that, if this version of the patchset allows non-init
>>>>>userns to unshare cgroupns, then the issue of what permission is
>>>>>needed to lock the cgroup hierarchy like that needs to be addressed,
>>>>>because unshare(CLONE_NEWUSER|CLONE_NEWCGROUP) will effectively pin
>>>>>the calling task with no permission required. Bolting on a fix later
>>>>>will be a mess.
>>>>
>>>> I imagine the pinning would be like the userns.
>>>>
>>>> Ah but there is a potentially serious issue with the pinning.
>>>> With pinning we can make it impossible for root to move us to a different cgroup.
>>>>
>>>> I am not certain how serious that is but it bears thinking about.
>>>> If we don't implement pinning we should be able to implent everything with just filesystem mount options, and no new namespace required.
>>>>
>>>> Sigh.
>>>>
>>>> I am too tired tonight to see the end game in this.
>>>
>>> Possible solution:
>>>
>>> Ditch the pinning. That is, if you're outside a cgroupns (or you have
>>> a non-ns-confined cgroupfs mounted), then you can move a task in a
>>> cgroupns outside of its root cgroup. If you do this, then the task
>>> thinks its cgroup is something like "../foo" or "../../foo".
>>
>> Of the possible solutions that seems attractive to me, simply because
>> we sometimes want to allow clever things to occur.
>>
>> Does anyone know of a reason (beyond pretty printing) why we need
>> cgroupns to restrict the subset of cgroups processes can be in?
>>
>> I would expect permissions on the cgroup directories themselves, and
>> limited visiblilty would be (in general) to achieve the desired
>> visiblity.
>
> This makes the security impact of cgroupns very easy to understand,
> right? Because there really won't be any -- cgroupns only affects
> reads from /proc and what cgroupfs shows, but it doesn't change any
> actual cgroups, nor does it affect any cgroup *changes*.
It seems like what we have described is chcgrouproot aka chroot for
cgroups. At which point I think there are potentially similar security
issues as for chroot. Can we confuse a setuid root process if we make
it's cgroup names look different.
Of course the confusing root concern is handled by the usual namespace
security checks that are already present.
I do wonder if we think of this as chcgrouproot if there is a simpler
implementation.
>>> While we're at it, consider making setns for a cgroupns *not* change
>>> the caller's cgroup. Is there any reason it really needs to?
>>
>> setns doesn't but nsenter is going to need to change the cgroup
>> if the pinning requirement is kept. nsenenter is going to want to
>> change the cgroup if the pinning requirement is dropped.
>>
>
> It seems easy enough for nsenter to change the cgroup all by itself.
Again. I don't think anyone has suggested or implemented anything
different.
Eric
^ permalink raw reply
* Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
From: Andy Lutomirski @ 2014-10-21 5:49 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux API, Linux Containers, Serge Hallyn,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ingo Molnar,
Tejun Heo, cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87lhoayo59.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
On Mon, Oct 20, 2014 at 10:42 PM, Eric W. Biederman
<ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>
>> On Mon, Oct 20, 2014 at 9:49 PM, Eric W. Biederman
>> <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes:
>>>> Possible solution:
>>>>
>>>> Ditch the pinning. That is, if you're outside a cgroupns (or you have
>>>> a non-ns-confined cgroupfs mounted), then you can move a task in a
>>>> cgroupns outside of its root cgroup. If you do this, then the task
>>>> thinks its cgroup is something like "../foo" or "../../foo".
>>>
>>> Of the possible solutions that seems attractive to me, simply because
>>> we sometimes want to allow clever things to occur.
>>>
>>> Does anyone know of a reason (beyond pretty printing) why we need
>>> cgroupns to restrict the subset of cgroups processes can be in?
>>>
>>> I would expect permissions on the cgroup directories themselves, and
>>> limited visiblilty would be (in general) to achieve the desired
>>> visiblity.
>>
>> This makes the security impact of cgroupns very easy to understand,
>> right? Because there really won't be any -- cgroupns only affects
>> reads from /proc and what cgroupfs shows, but it doesn't change any
>> actual cgroups, nor does it affect any cgroup *changes*.
>
> It seems like what we have described is chcgrouproot aka chroot for
> cgroups. At which point I think there are potentially similar security
> issues as for chroot. Can we confuse a setuid root process if we make
> it's cgroup names look different.
>
> Of course the confusing root concern is handled by the usual namespace
> security checks that are already present.
I think that the chroot issues are mostly in two categories: setuid
confusion (not an issue here as you described) and chroot escapes.
cgroupns escapes aren't a big deal, I think -- admins should deny the
confined task the right to write to cgroupfs outside its hierarchy, by
setting cgroupfs permissions appropriately and/or avoiding mounting
cgroupfs outside the hierarchy.
>
> I do wonder if we think of this as chcgrouproot if there is a simpler
> implementation.
Could be. I'll defer to Aditya for that one.
>
>>>> While we're at it, consider making setns for a cgroupns *not* change
>>>> the caller's cgroup. Is there any reason it really needs to?
>>>
>>> setns doesn't but nsenter is going to need to change the cgroup
>>> if the pinning requirement is kept. nsenenter is going to want to
>>> change the cgroup if the pinning requirement is dropped.
>>>
>>
>> It seems easy enough for nsenter to change the cgroup all by itself.
>
> Again. I don't think anyone has suggested or implemented anything
> different.
The current patchset seems to punt on this decision by just failing
the setns call if the caller is outside the cgroup in question.
--Andy
^ permalink raw reply
* Re: preadv2/pwritev2 rename
From: Michael Kerrisk (man-pages) @ 2014-10-21 6:42 UTC (permalink / raw)
To: Milosz Tanski; +Cc: Christoph Hellwig, Jeff Moyer, Linux API
In-Reply-To: <CANP1eJFfGE53PTGR4XYGmr=HBLLdOByJj+GbkQB6+JSdrgtvFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hello Milosz,
On Mon, Oct 20, 2014 at 11:52 PM, Milosz Tanski <milosz-B5zB6C1i6pkAvxtiuMwx3w@public.gmane.org> wrote:
> Christoph and/or Jeff,
>
> I updated the patch for 3.18-rc1 and I'm going to resend it as non-RFC
> as I didn't get comments last time.
>
> I only have one stupid question... I'm going to rename the calls to
> preadv6 and pwritev6 (so it's more like the other syscalls: dup3,
> accept4, eventfd2) but I'm not sure if i should call it preadv5 or
> pwritev6 since the offset argument is split into two different
> arguments (upper and lower part).
It's points like this that show exactly why naming system calls after
the number of their arguments is a very bad idea[1]. Please don't do
it. pwritev2() and preadv2() are not pretty either, but are marginally
better. pwritev_fl() and preadv_fl() (or simialr) might also be okay,
I guess.
> Also, In our application we were able to get about 20%-30% reduction
> in response time when using this before queuing in a IO thread pool on
> the read path. It's a pretty nice win in the real world.
Cheers,
Michael
http://blog.man7.org/2014/02/system-call-naming-and-numbering.html
^ permalink raw reply
* Re: preadv2/pwritev2 rename
From: Michael Kerrisk (man-pages) @ 2014-10-21 6:53 UTC (permalink / raw)
To: Milosz Tanski; +Cc: Christoph Hellwig, Jeff Moyer, Linux API
In-Reply-To: <CAKgNAkjwJ6SH_j_Op+u+p4-0mN-cBVE24Wrt4SyhAiG1MMF9tA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Oct 21, 2014 at 8:42 AM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello Milosz,
>
> On Mon, Oct 20, 2014 at 11:52 PM, Milosz Tanski <milosz-B5zB6C1i6pkAvxtiuMwx3w@public.gmane.org> wrote:
>> Christoph and/or Jeff,
>>
>> I updated the patch for 3.18-rc1 and I'm going to resend it as non-RFC
>> as I didn't get comments last time.
>>
>> I only have one stupid question... I'm going to rename the calls to
>> preadv6 and pwritev6 (so it's more like the other syscalls: dup3,
>> accept4, eventfd2) but I'm not sure if i should call it preadv5 or
>> pwritev6 since the offset argument is split into two different
>> arguments (upper and lower part).
>
> It's points like this that show exactly why naming system calls after
> the number of their arguments is a very bad idea[1]. Please don't do
> it. pwritev2() and preadv2() are not pretty either, but are marginally
> better. pwritev_fl() and preadv_fl() (or simialr) might also be okay,
> I guess.
>
>> Also, In our application we were able to get about 20%-30% reduction
>> in response time when using this before queuing in a IO thread pool on
>> the read path. It's a pretty nice win in the real world.
>
> Cheers,
>
> Michael
>
> http://blog.man7.org/2014/02/system-call-naming-and-numbering.html
Also, please ensure that future iterations of these patches CC
linux-api@ as per Documentation/SubmitChecklist. The past ones did
not, and so this is the first mention of these system calls that I
happened to see.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 2/2] fs: Support compiling out sendfile
From: Christoph Hellwig @ 2014-10-21 7:51 UTC (permalink / raw)
To: josh-iaAMLnmF4UmaiuxdJuQwMA
Cc: Pieter Smith, Alexander Viro, Andrew Morton, Eric Paris,
Matt Turner, Michal Hocko, Paul E. McKenney, Fabian Frederick,
Tejun Heo, ?????????, Luis R. Rodriguez, Peter Foley,
Konstantin Khlebnikov, Eric W. Biederman, H. Peter Anvin,
Oleg Nesterov, Andy Lutomirski, David Herrmann, Kees Cook,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, open list,
open list:ABI/API
In-Reply-To: <20141020222421.GE8929@cloud>
On Mon, Oct 20, 2014 at 03:24:22PM -0700, josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org wrote:
> On Mon, Oct 20, 2014 at 11:48:37PM +0200, Pieter Smith wrote:
> > Many embedded systems will not need this syscall, and omitting it
> > saves space. Add a new EXPERT config option CONFIG_SENDFILE_SYSCALL
> > (default y) to support compiling it out.
>
> Nice work, thanks!
>
> If there are no objections, and nobody has a tree they'd rather carry
> this through, I'll take the series through the tiny tree when it's ready
> to merge.
I think it's rather pointless - there is very little sendfile code,
so you'd rather want to disable splice.
^ permalink raw reply
* Re: [PATCH 2/2] fs: Support compiling out sendfile
From: Josh Triplett @ 2014-10-21 9:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Pieter Smith, Alexander Viro, Andrew Morton, Eric Paris,
Matt Turner, Michal Hocko, Paul E. McKenney, Fabian Frederick,
Tejun Heo, ?????????, Luis R. Rodriguez, Peter Foley,
Konstantin Khlebnikov, Eric W. Biederman, H. Peter Anvin,
Oleg Nesterov, Andy Lutomirski, David Herrmann, Kees Cook,
linux-fsdevel, open list, open list:ABI/API
In-Reply-To: <20141021075154.GB10020@infradead.org>
On Tue, Oct 21, 2014 at 12:51:54AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 20, 2014 at 03:24:22PM -0700, josh@joshtriplett.org wrote:
> > On Mon, Oct 20, 2014 at 11:48:37PM +0200, Pieter Smith wrote:
> > > Many embedded systems will not need this syscall, and omitting it
> > > saves space. Add a new EXPERT config option CONFIG_SENDFILE_SYSCALL
> > > (default y) to support compiling it out.
> >
> > Nice work, thanks!
> >
> > If there are no objections, and nobody has a tree they'd rather carry
> > this through, I'll take the series through the tiny tree when it's ready
> > to merge.
>
> I think it's rather pointless - there is very little sendfile code,
> so you'd rather want to disable splice.
That's the plan, but since sendfile depends on some of the splice bits,
sendfile needs to be optional as well; SENDFILE_SYSCALL will then select
SPLICE_SYSCALLS.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 2/2] fs: Support compiling out sendfile
From: Christoph Hellwig @ 2014-10-21 9:13 UTC (permalink / raw)
To: Josh Triplett
Cc: Christoph Hellwig, Pieter Smith, Alexander Viro, Andrew Morton,
Eric Paris, Matt Turner, Michal Hocko, Paul E. McKenney,
Fabian Frederick, Tejun Heo, ?????????, Luis R. Rodriguez,
Peter Foley, Konstantin Khlebnikov, Eric W. Biederman,
H. Peter Anvin, Oleg Nesterov, Andy Lutomirski, David Herrmann,
Kees Cook, linux-fsdevel, open list, open list:ABI/API
In-Reply-To: <20141021090421.GB17604@thin>
On Tue, Oct 21, 2014 at 02:04:22AM -0700, Josh Triplett wrote:
> That's the plan, but since sendfile depends on some of the splice bits,
> sendfile needs to be optional as well; SENDFILE_SYSCALL will then select
> SPLICE_SYSCALLS.
Just include sendfile with the splice syscalls - we don't really need a
config option for every obscure syscall.
^ permalink raw reply
* Re: [PATCH 2/2] fs: Support compiling out sendfile
From: Josh Triplett @ 2014-10-21 9:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Pieter Smith, Alexander Viro, Andrew Morton, Eric Paris,
Matt Turner, Michal Hocko, Paul E. McKenney, Fabian Frederick,
Tejun Heo, ?????????, Luis R. Rodriguez, Peter Foley,
Konstantin Khlebnikov, Eric W. Biederman, H. Peter Anvin,
Oleg Nesterov, Andy Lutomirski, David Herrmann, Kees Cook,
linux-fsdevel, open list, open list:ABI/API
In-Reply-To: <20141021091356.GA26330@infradead.org>
On Tue, Oct 21, 2014 at 02:13:56AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 21, 2014 at 02:04:22AM -0700, Josh Triplett wrote:
> > That's the plan, but since sendfile depends on some of the splice bits,
> > sendfile needs to be optional as well; SENDFILE_SYSCALL will then select
> > SPLICE_SYSCALLS.
>
> Just include sendfile with the splice syscalls - we don't really need a
> config option for every obscure syscall.
No objection here. Pieter, since you're planning to remove splice
anyway, can you just fold the two together under the same Kconfig
option? That should simplify the patch series, since you won't need to
split the two.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Pavel Machek @ 2014-10-21 10:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Anup Patel, Linux API, lkml, Arve Hjønnevåg,
John Stultz, Michael Kerrisk (man-pages), Rebecca Schultz Zavin,
Santosh Shilimkar, Sumit Semwal, Christoffer Dall
In-Reply-To: <20141016231457.GB13592@kroah.com>
On Fri 2014-10-17 01:14:57, Greg Kroah-Hartman wrote:
> On Thu, Oct 16, 2014 at 04:18:02PM +0200, Michael Kerrisk (man-pages) wrote:
> > On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > > The Android binder code has been "stable" for many years now. No matter
> > > what comes in the future, we are going to have to support this API, so
> > > might as well move it to the "real" part of the kernel as there's no
> > > real work that needs to be done to the existing code.
> >
> > Where does one find the canonical documentation of the user-space API?
>
> There really is only one "canonical" thing, and that is in the libbinder
> code in the Android userspace repository. And it's not really
> "documentation" so much as, "a C file that interacts with the ioctls in
> the binder kernel code" :(
>
> Think of this as just a random character driver with some funny ioctls
> that will never get really documented as there is only one user of it.
This is not random character driver, it is communication mechanism. It
should _not_ be a character driver.
And it should really be documented.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 2/5] mfd: max77693: Map charger device to its own of_node
From: Lee Jones @ 2014-10-21 10:17 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel, linux-pm, Samuel Ortiz, linux-api, devicetree,
Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz
In-Reply-To: <1413817781.11298.0.camel@AMDC1943>
On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> On pon, 2014-10-20 at 16:06 +0100, Lee Jones wrote:
> > On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> >
> > > Add a "maxim,max77693-charger" of_compatible to the mfd_cell so the MFD
> > > child device (the charger) will have its own of_node set. This will be
> > > used by the max77693 charger driver in next patches to obtain battery
> > > configuration from DTS.
> > >
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > ---
> > > drivers/mfd/max77693.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> > > index cf008f45968c..2277a11b6629 100644
> > > --- a/drivers/mfd/max77693.c
> > > +++ b/drivers/mfd/max77693.c
> > > @@ -43,7 +43,10 @@
> > >
> > > static const struct mfd_cell max77693_devs[] = {
> > > { .name = "max77693-pmic", },
> > > - { .name = "max77693-charger", },
> > > + {
> > > + .name = "max77693-charger",
> > > + .of_compatible = "maxim,max77693-charger",
> > > + },
> > > { .name = "max77693-muic", },
> > > { .name = "max77693-haptic", },
> > > {
> >
> > I'm guessing this can be applied separately?
>
> Yes. Patch 3/5 also (it adds necessary defines and symbols for
> chargers).
I'll take this patch, but I think 3/5 is required by some of the other
patches in the set, no?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 2/5] mfd: max77693: Map charger device to its own of_node
From: Lee Jones @ 2014-10-21 10:17 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel, linux-pm, Samuel Ortiz, linux-api, devicetree,
Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz
In-Reply-To: <1413808489-3524-3-git-send-email-k.kozlowski@samsung.com>
On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> Add a "maxim,max77693-charger" of_compatible to the mfd_cell so the MFD
> child device (the charger) will have its own of_node set. This will be
> used by the max77693 charger driver in next patches to obtain battery
> configuration from DTS.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
> drivers/mfd/max77693.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Applied, thanks.
> diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> index cf008f45968c..2277a11b6629 100644
> --- a/drivers/mfd/max77693.c
> +++ b/drivers/mfd/max77693.c
> @@ -43,7 +43,10 @@
>
> static const struct mfd_cell max77693_devs[] = {
> { .name = "max77693-pmic", },
> - { .name = "max77693-charger", },
> + {
> + .name = "max77693-charger",
> + .of_compatible = "maxim,max77693-charger",
> + },
> { .name = "max77693-muic", },
> { .name = "max77693-haptic", },
> {
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 2/5] mfd: max77693: Map charger device to its own of_node
From: Krzysztof Kozlowski @ 2014-10-21 10:20 UTC (permalink / raw)
To: Lee Jones
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA, Samuel Ortiz,
linux-api-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
In-Reply-To: <20141021101705.GF26842@x1>
On wto, 2014-10-21 at 11:17 +0100, Lee Jones wrote:
> On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
>
> > On pon, 2014-10-20 at 16:06 +0100, Lee Jones wrote:
> > > On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> > >
> > > > Add a "maxim,max77693-charger" of_compatible to the mfd_cell so the MFD
> > > > child device (the charger) will have its own of_node set. This will be
> > > > used by the max77693 charger driver in next patches to obtain battery
> > > > configuration from DTS.
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > ---
> > > > drivers/mfd/max77693.c | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> > > > index cf008f45968c..2277a11b6629 100644
> > > > --- a/drivers/mfd/max77693.c
> > > > +++ b/drivers/mfd/max77693.c
> > > > @@ -43,7 +43,10 @@
> > > >
> > > > static const struct mfd_cell max77693_devs[] = {
> > > > { .name = "max77693-pmic", },
> > > > - { .name = "max77693-charger", },
> > > > + {
> > > > + .name = "max77693-charger",
> > > > + .of_compatible = "maxim,max77693-charger",
> > > > + },
> > > > { .name = "max77693-muic", },
> > > > { .name = "max77693-haptic", },
> > > > {
> > >
> > > I'm guessing this can be applied separately?
> >
> > Yes. Patch 3/5 also (it adds necessary defines and symbols for
> > chargers).
>
> I'll take this patch, but I think 3/5 is required by some of the other
> patches in the set, no?
Yes, it is required by 4/5 (charger driver).
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/5] mfd: max77693: Map charger device to its own of_node
From: Lee Jones @ 2014-10-21 10:27 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA, Samuel Ortiz,
linux-api-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
In-Reply-To: <1413886845.26980.18.camel@AMDC1943>
On Tue, 21 Oct 2014, Krzysztof Kozlowski wrote:
> On wto, 2014-10-21 at 11:17 +0100, Lee Jones wrote:
> > On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> >
> > > On pon, 2014-10-20 at 16:06 +0100, Lee Jones wrote:
> > > > On Mon, 20 Oct 2014, Krzysztof Kozlowski wrote:
> > > >
> > > > > Add a "maxim,max77693-charger" of_compatible to the mfd_cell so the MFD
> > > > > child device (the charger) will have its own of_node set. This will be
> > > > > used by the max77693 charger driver in next patches to obtain battery
> > > > > configuration from DTS.
> > > > >
> > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > ---
> > > > > drivers/mfd/max77693.c | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> > > > > index cf008f45968c..2277a11b6629 100644
> > > > > --- a/drivers/mfd/max77693.c
> > > > > +++ b/drivers/mfd/max77693.c
> > > > > @@ -43,7 +43,10 @@
> > > > >
> > > > > static const struct mfd_cell max77693_devs[] = {
> > > > > { .name = "max77693-pmic", },
> > > > > - { .name = "max77693-charger", },
> > > > > + {
> > > > > + .name = "max77693-charger",
> > > > > + .of_compatible = "maxim,max77693-charger",
> > > > > + },
> > > > > { .name = "max77693-muic", },
> > > > > { .name = "max77693-haptic", },
> > > > > {
> > > >
> > > > I'm guessing this can be applied separately?
> > >
> > > Yes. Patch 3/5 also (it adds necessary defines and symbols for
> > > chargers).
> >
> > I'll take this patch, but I think 3/5 is required by some of the other
> > patches in the set, no?
>
> Yes, it is required by 4/5 (charger driver).
Okay, so it must go in with the others then.
I need Acks for that.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Pavel Machek @ 2014-10-21 10:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
Cc: John Stultz, lkml, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
Linux API, Santosh Shilimkar, Arve Hjønnevåg,
Sumit Semwal, Rebecca Schultz Zavin, Christoffer Dall, Anup Patel
In-Reply-To: <20141016231221.GA13592-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
> On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> > On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> > <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > > From: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > Are the Android guys comfortable with the ABI stability rules they'll
> > now face?
>
> Just because something is in staging, doesn't mean you don't have to
> follow the same ABI stability rules as the rest of the kernel. If a
> change had happened to this code that broke userspace in the past, I
> would have reverted it. So this should not be anything different from
> what has been happening inthe past.
Actually, there's big difference.
If Al Viro changes core filesystem in a way that breaks
staging/binder, binder is broken, and if it can't be fixed... well it
can't be fixed.
If Al Viro changes core filesystem in a way that breaks
drivers/binder, Al's change is going to be reverted.
It is really hard to review without API documentation. Normally, API
documentation is required for stuff like this.
For example: does it add new files in /proc?
Given that it is stable, can we get rid of binder_debug() and
especially BINDER_DEBUG_ENTRY stuff?
Checkpatch warns about 98 too long lines. Some of them could be fixed
easily.
This looks scary:
trace_binder_transaction_fd(t, fp->handle,
target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
" fd %d -> %d\n",
fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
Could binder_transcation() be split to smaller functions according to
CodingStyle? 17 goto targets at the end of function are not exactly
easy to read.
ginder_thread_read/write also needs splitting.
binder_ioctl_write_read: just use direct return, no need to goto out
if it just returns.
proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
mutex_unlock(&binder_mmap_lock);
#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
while (CACHE_COLOUR((vma->vm_start ^
(uint32_t)proc->buffer))) {
Should this be (uintptr_t)?
/*pr_info("binder_mmap: %d %lx-%lx maps %p\n",
Delete the code, don't comment it out. It is on more than one place.
static void print_binder_thread(struct seq_file *m,
struct binder_thread *thread,
int print_always)
{
struct binder_transaction *t;
struct binder_work *w;
size_t start_pos = m->count;
size_t header_pos;
seq_printf(m, " thread %d: l %02x\n", thread->pid,
thread->looper);
header_pos = m->count;
t = thread->transaction_stack;
while (t) {
if (t->from == thread) {
print_binder_transaction(m,
"
outgoing transaction", t);
t = t->from_parent;
Is anyone depending on the debugfs files? Can it be deleted?
Code indentation is "interesting" in binder_thread_read(). See the "}
break;" lines. {}s should not be needed...?
I don't think this code would get merged if it was submitted for
normal inclusion in kernel. I don't think it is good idea to push it
through the back door, without documenting what it does and without
patches even going to the lists.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Christoph Hellwig @ 2014-10-21 10:46 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Anup Patel,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Arve Hj?nnev?g,
Santosh Shilimkar, Rebecca Schultz Zavin, John Stultz,
Sumit Semwal, Christoffer Dall
In-Reply-To: <20141019220450.GB3780-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Mon, Oct 20, 2014 at 06:04:50AM +0800, Greg Kroah-Hartman wrote:
> There is now work to resolve the interface, it requires someone who has
> the rights to push to Android userspace. But that is going to be a
> "total rewrite", and until then, this code needs to be used, no matter
> how much we hate this.
It helps to qualify why it absolutely has to, and why this is different
from other interfaces we haven't merged. Is this the last building
block to run upstream Linux on a common Android device out of the box
without needing any patches or out of tree drivers? Is there any other
good reason I might have missed.
To convince other people that merging a piece like this absolutely needs
to get merged I'd suggest you start with presenting factual argument,
and then let the broader audience weight their merrits.
I think with the known problems of the code, and the fact that the
real user ABI is a library anyway the stakes are quite high here.
So as a start please prepare a list of arguments, a detailed description
of the ABI, and post a proper patch (not a move) that suggests adding
this driver to all the relevant lists (most importantly linux-fsdevel
and linux-api) so that people with the right experience can review it.
^ permalink raw reply
* Re: [PATCH 2/5] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
From: Antonios Motakis @ 2014-10-21 12:17 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm-arm, Linux IOMMU, Will Deacon,
VirtualOpenSystems Technical Team, Christoffer Dall, Eric Auger,
Kim Phillips, Marc Zyngier, open list:VFIO DRIVER,
open list:ABI/API, open list
In-Reply-To: <1413840583.4202.111.camel@ul30vt.home>
On Mon, Oct 20, 2014 at 11:29 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2014-10-13 at 15:09 +0200, Antonios Motakis wrote:
>> We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
>> and expose its availability via the capability VFIO_DMA_NOEXEC_IOMMU.
>> This way the user can control whether the XN flag will be set on the
>> requested mappings. The IOMMU_NOEXEC flag needs to be available for all
>> the IOMMUs of the container used.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>> include/uapi/linux/vfio.h | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 6612974..111b5e8 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -29,6 +29,7 @@
>> * capability is subject to change as groups are added or removed.
>> */
>> #define VFIO_DMA_CC_IOMMU 4
>> +#define VFIO_DMA_NOEXEC_IOMMU 5
>>
>> /* Check if EEH is supported */
>> #define VFIO_EEH 5
> ^^
> 5 is still already used. Feel free to convert to enum so we stop making
> this mistake.
Oops :) will do.
>
>> @@ -401,6 +402,7 @@ struct vfio_iommu_type1_dma_map {
>> __u32 flags;
>> #define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */
>> #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */
>> +#define VFIO_DMA_MAP_FLAG_NOEXEC (1 << 2) /* not executable from device */
>> __u64 vaddr; /* Process virtual address */
>> __u64 iova; /* IO virtual address */
>> __u64 size; /* Size of mapping (bytes) */
>
>
>
--
Antonios Motakis
Virtual Open Systems
^ permalink raw reply
* Re: [PATCH 2/5] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag
From: Antonios Motakis @ 2014-10-21 12:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: open list:VFIO DRIVER, Eric Auger, Marc Zyngier,
open list:ABI/API, Will Deacon, open list, Linux IOMMU,
VirtualOpenSystems Technical Team, kvm-arm, Christoffer Dall
In-Reply-To: <CALCETrWzxjpKrou6J63_T75x=ZEGWCGbc4KEWT_AMvzSQNn1eg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Oct 20, 2014 at 11:37 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Mon, Oct 13, 2014 at 6:09 AM, Antonios Motakis
> <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org> wrote:
>> We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
>> and expose its availability via the capability VFIO_DMA_NOEXEC_IOMMU.
>> This way the user can control whether the XN flag will be set on the
>> requested mappings. The IOMMU_NOEXEC flag needs to be available for all
>> the IOMMUs of the container used.
>
> Since you sent this to the linux-api list, I'll bite: what's the XN
> flag? I know what PROT_EXEC does when you mmap something, and I
> presume that vfio is mmappable, but I don't actually have any clue
> what this patch does.
>
> I assume that this does not have anything to do with a non-CPU DMA
> master executing code in main memory, because that makes rather little
> sense. (Or maybe it really does, in which case: weird.)
It does actually. For example, the ARM PL330 DMA controller will fetch
from memory code with DMA instructions, and it will respect this flag.
It is not code that can be executed on the CPU of course, but it is
executable on the DMAC.
>
> --Andy
--
Antonios Motakis
Virtual Open Systems
^ permalink raw reply
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Arnd Bergmann @ 2014-10-21 14:12 UTC (permalink / raw)
To: Pavel Machek
Cc: devel, Anup Patel, Greg Kroah-Hartman, lkml,
Arve Hjønnevåg, John Stultz, viro, Linux API,
Rebecca Schultz Zavin, Santosh Shilimkar, Sumit Semwal,
Christoffer Dall
In-Reply-To: <20141021103622.GB23161@amd>
On Tuesday 21 October 2014 12:36:22 Pavel Machek wrote:
> On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
> > On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> > > On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> > > Are the Android guys comfortable with the ABI stability rules they'll
> > > now face?
> >
> > Just because something is in staging, doesn't mean you don't have to
> > follow the same ABI stability rules as the rest of the kernel. If a
> > change had happened to this code that broke userspace in the past, I
> > would have reverted it. So this should not be anything different from
> > what has been happening inthe past.
>
> Actually, there's big difference.
>
> If Al Viro changes core filesystem in a way that breaks
> staging/binder, binder is broken, and if it can't be fixed... well it
> can't be fixed.
>
> If Al Viro changes core filesystem in a way that breaks
> drivers/binder, Al's change is going to be reverted.
One might have argued that we'd have to do that already, but the reasons
for doing that with binder in the main kernel are certainly stronger.
> It is really hard to review without API documentation. Normally, API
> documentation is required for stuff like this.
>
> For example: does it add new files in /proc?
>
> Given that it is stable, can we get rid of binder_debug() and
> especially BINDER_DEBUG_ENTRY stuff?
Good point. We require documentation for every single sysfs attribute
that gets added to a driver (some escape the review, but that doesn't
change the rule), so we should not make an exception for a new procfs
file here.
> Checkpatch warns about 98 too long lines. Some of them could be fixed
> easily.
I don't think this should be an argument.
> This looks scary:
>
> trace_binder_transaction_fd(t, fp->handle,
> target_fd);
> binder_debug(BINDER_DEBUG_TRANSACTION,
> " fd %d -> %d\n",
> fp->handle, target_fd);
> /* TODO: fput? */
> fp->handle = target_fd;
> } break;
>
> Could binder_transcation() be split to smaller functions according to
> CodingStyle? 17 goto targets at the end of function are not exactly
> easy to read.
>
> ginder_thread_read/write also needs splitting.
Yes, in principle, but this is still a detail that would mainly serve
to simplify review. The problem is more the lack of review and
documentation of the API.
> binder_ioctl_write_read: just use direct return, no need to goto out
> if it just returns.
details, and a lot of people actually like the style used there.
> proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
> mutex_unlock(&binder_mmap_lock);
>
> #ifdef CONFIG_CPU_CACHE_VIPT
> if (cache_is_vipt_aliasing()) {
> while (CACHE_COLOUR((vma->vm_start ^
> (uint32_t)proc->buffer))) {
>
> Should this be (uintptr_t)?
It should probably call an architecture specific helper.
Arnd
^ permalink raw reply
* Re: [PATCH v8 02/18] vfio: platform: probe to devices on the platform bus
From: Alex Williamson @ 2014-10-21 16:17 UTC (permalink / raw)
To: Antonios Motakis
Cc: open list:VFIO DRIVER, eric.auger-QSEj5FYQhm4dnm+yROfE0A,
marc.zyngier-5wv7dgnIgG8, open list:ABI/API,
will.deacon-5wv7dgnIgG8, open list,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <1413205825-6370-3-git-send-email-a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
> Driver to bind to Linux platform devices, and callbacks to discover their
> resources to be used by the main VFIO PLATFORM code.
>
> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
> drivers/vfio/platform/vfio_platform.c | 107 ++++++++++++++++++++++++++++++++++
> include/uapi/linux/vfio.h | 1 +
> 2 files changed, 108 insertions(+)
> create mode 100644 drivers/vfio/platform/vfio_platform.c
>
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> new file mode 100644
> index 0000000..baeb7da
> --- /dev/null
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -0,0 +1,107 @@
> +/*
> + * Copyright (C) 2013 - Virtual Open Systems
> + * Author: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/eventfd.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq.h>
> +
> +#include "vfio_platform_private.h"
> +
> +#define DRIVER_VERSION "0.8"
> +#define DRIVER_AUTHOR "Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>"
> +#define DRIVER_DESC "VFIO for platform devices - User Level meta-driver"
> +
> +/* probing devices from the linux platform bus */
> +
> +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> + int num)
> +{
> + struct platform_device *dev = (struct platform_device *) vdev->opaque;
> + int i;
> +
> + for (i = 0; i < dev->num_resources; i++) {
> + struct resource *r = &dev->resource[i];
> +
> + if (resource_type(r) & (IORESOURCE_MEM|IORESOURCE_IO)) {
> + num--;
> +
> + if (!num)
> + return r;
Has this been tested? What happens when we call this with num = 0?
> + }
> + }
> + return NULL;
> +}
> +
> +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
> +{
> + struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> +
> + return platform_get_irq(pdev, i);
> +}
> +
> +
> +static int vfio_platform_probe(struct platform_device *pdev)
> +{
> + struct vfio_platform_device *vdev;
> + int ret;
> +
> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> + if (!vdev)
> + return -ENOMEM;
> +
> + vdev->opaque = (void *) pdev;
> + vdev->name = pdev->name;
> + vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
> + vdev->get_resource = get_platform_resource;
> + vdev->get_irq = get_platform_irq;
> +
> + ret = vfio_platform_probe_common(vdev, &pdev->dev);
> + if (ret)
> + kfree(vdev);
> +
> + return ret;
> +}
> +
> +static int vfio_platform_remove(struct platform_device *pdev)
> +{
> + return vfio_platform_remove_common(&pdev->dev);
> +}
> +
> +static struct platform_driver vfio_platform_driver = {
> + .probe = vfio_platform_probe,
> + .remove = vfio_platform_remove,
> + .driver = {
> + .name = "vfio-platform",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(vfio_platform_driver);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 111b5e8..aca6d3e 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -158,6 +158,7 @@ struct vfio_device_info {
> __u32 flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
> +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> __u32 num_regions; /* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
^ permalink raw reply
* Re: [PATCH v8 02/18] vfio: platform: probe to devices on the platform bus
From: Eric Auger @ 2014-10-21 16:37 UTC (permalink / raw)
To: Alex Williamson, Antonios Motakis
Cc: VFIO DRIVER, marc.zyngier-5wv7dgnIgG8, ABI/API,
will.deacon-5wv7dgnIgG8, open list,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <1413908269.4202.135.camel-85EaTFmN5p//9pzu0YdTqQ@public.gmane.org>
On 10/21/2014 06:17 PM, Alex Williamson wrote:
> On Mon, 2014-10-13 at 15:10 +0200, Antonios Motakis wrote:
>> Driver to bind to Linux platform devices, and callbacks to discover their
>> resources to be used by the main VFIO PLATFORM code.
>>
>> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> ---
>> drivers/vfio/platform/vfio_platform.c | 107 ++++++++++++++++++++++++++++++++++
>> include/uapi/linux/vfio.h | 1 +
>> 2 files changed, 108 insertions(+)
>> create mode 100644 drivers/vfio/platform/vfio_platform.c
>>
>> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
>> new file mode 100644
>> index 0000000..baeb7da
>> --- /dev/null
>> +++ b/drivers/vfio/platform/vfio_platform.c
>> @@ -0,0 +1,107 @@
>> +/*
>> + * Copyright (C) 2013 - Virtual Open Systems
>> + * Author: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/notifier.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vfio.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/irq.h>
>> +
>> +#include "vfio_platform_private.h"
>> +
>> +#define DRIVER_VERSION "0.8"
>> +#define DRIVER_AUTHOR "Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>"
>> +#define DRIVER_DESC "VFIO for platform devices - User Level meta-driver"
>> +
>> +/* probing devices from the linux platform bus */
>> +
>> +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
>> + int num)
>> +{
>> + struct platform_device *dev = (struct platform_device *) vdev->opaque;
>> + int i;
>> +
>> + for (i = 0; i < dev->num_resources; i++) {
>> + struct resource *r = &dev->resource[i];
>> +
>> + if (resource_type(r) & (IORESOURCE_MEM|IORESOURCE_IO)) {
>> + num--;
>> +
>> + if (!num)
>> + return r;
>
> Has this been tested? What happens when we call this with num = 0?
Yep. I confirm I enter that case with my xgmac where the IORESOURCE_MEM
is the 1st resource. I Just ended to the same cause ;-)
Best Regards
Eric
>
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
>> +{
>> + struct platform_device *pdev = (struct platform_device *) vdev->opaque;
>> +
>> + return platform_get_irq(pdev, i);
>> +}
>> +
>> +
>> +static int vfio_platform_probe(struct platform_device *pdev)
>> +{
>> + struct vfio_platform_device *vdev;
>> + int ret;
>> +
>> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
>> + if (!vdev)
>> + return -ENOMEM;
>> +
>> + vdev->opaque = (void *) pdev;
>> + vdev->name = pdev->name;
>> + vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
>> + vdev->get_resource = get_platform_resource;
>> + vdev->get_irq = get_platform_irq;
>> +
>> + ret = vfio_platform_probe_common(vdev, &pdev->dev);
>> + if (ret)
>> + kfree(vdev);
>> +
>> + return ret;
>> +}
>> +
>> +static int vfio_platform_remove(struct platform_device *pdev)
>> +{
>> + return vfio_platform_remove_common(&pdev->dev);
>> +}
>> +
>> +static struct platform_driver vfio_platform_driver = {
>> + .probe = vfio_platform_probe,
>> + .remove = vfio_platform_remove,
>> + .driver = {
>> + .name = "vfio-platform",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +module_platform_driver(vfio_platform_driver);
>> +
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 111b5e8..aca6d3e 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -158,6 +158,7 @@ struct vfio_device_info {
>> __u32 flags;
>> #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
>> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
>> +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
>> __u32 num_regions; /* Max region index + 1 */
>> __u32 num_irqs; /* Max IRQ index + 1 */
>> };
>
>
>
^ permalink raw reply
* [PATCH V2] kernel, add bug_on_warn
From: Prarit Bhargava @ 2014-10-21 16:47 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Prarit Bhargava, Jonathan Corbet, Andrew Morton, Rusty Russell,
H. Peter Anvin, Andi Kleen, Masami Hiramatsu, Fabian Frederick,
vgoyal-H+wXaHxf7aLQT0dZR+AlfA,
isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-api-u79uwXL29TY76Z2rM5mHXA
There have been several times where I have had to rebuild a kernel to
cause a panic when hitting a WARN() in the code in order to get a crash
dump from a system. Sometimes this is easy to do, other times (such as
in the case of a remote admin) it is not trivial to send new images to the
user.
A much easier method would be a switch to change the WARN() over to a
BUG(). This makes debugging easier in that I can now test the actual
image the WARN() was seen on and I do not have to engage in remote
debugging.
This patch adds a bug_on_warn kernel parameter, which calls BUG() in the
warn_slowpath_common() path. The function will still print out the
location of the warning.
An example of the bug_on_warn output:
The first line below is from the WARN_ON() to output the WARN_ON()'s location.
After that the new BUG() call is displayed.
WARNING: CPU: 27 PID: 3204 at
/home/rhel7/redhat/debug/dummy-module/dummy-module.c:25 init_dummy+0x28/0x30
[dummy_module]()
bug_on_warn set, calling BUG()...
------------[ cut here ]------------
kernel BUG at kernel/panic.c:434!
invalid opcode: 0000 [#1] SMP
Modules linked in: dummy_module(OE+) sg nfsv3 rpcsec_gss_krb5 nfsv4
dns_resolver nfs fscache cfg80211 rfkill x86_pkg_temp_thermal intel_powerclamp
coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul crc32c_intel
ghash_clmulni_intel igb iTCO_wdt aesni_intel iTCO_vendor_support lrw gf128mul
sb_edac ptp edac_core glue_helper lpc_ich ioatdma pcspkr ablk_helper pps_core
i2c_i801 mfd_core cryptd dca shpchp ipmi_si wmi ipmi_msghandler acpi_cpufreq
nfsd auth_rpcgss nfs_acl lockd grace sunrpc xfs libcrc32c sr_mod cdrom sd_mod
mgag200 syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper isci ttm
drm libsas ahci libahci scsi_transport_sas libata i2c_core dm_mirror
dm_region_hash dm_log dm_mod
CPU: 27 PID: 3204 Comm: insmod Tainted: G OE 3.17.0+ #19
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS
RMLSDP.86I.00.29.D696.1311111329 11/11/2013
task: ffff880034e75160 ti: ffff8807fc5ac000 task.ti: ffff8807fc5ac000
RIP: 0010:[<ffffffff81076b81>] [<ffffffff81076b81>] warn_slowpath_common+0xc1/0xd0
RSP: 0018:ffff8807fc5afc68 EFLAGS: 00010246
RAX: 0000000000000021 RBX: ffff8807fc5afcb0 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff88081efee5f8 RDI: ffff88081efee5f8
RBP: ffff8807fc5afc98 R08: 0000000000000096 R09: 0000000000000000
R10: 0000000000000711 R11: ffff8807fc5af93e R12: ffffffffa0424070
R13: 0000000000000019 R14: ffffffffa0423068 R15: 0000000000000009
FS: 00007f2d4b034740(0000) GS:ffff88081efe0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f2d4a99f3c0 CR3: 00000007fd88b000 CR4: 00000000001407e0
Stack:
ffff8807fc5afcb8 ffffffff8199f020 ffff88080e396160 0000000000000000
ffffffffa0423040 ffffffffa0425000 ffff8807fc5afd08 ffffffff81076be5
0000000000000008 ffffffffa0424053 ffff880700000018 ffff8807fc5afd18
Call Trace:
[<ffffffffa0423040>] ? dummy_greetings+0x40/0x40 [dummy_module]
[<ffffffff81076be5>] warn_slowpath_fmt+0x55/0x70
[<ffffffffa0423068>] init_dummy+0x28/0x30 [dummy_module]
[<ffffffff81002144>] do_one_initcall+0xd4/0x210
[<ffffffff811b52c2>] ? __vunmap+0xc2/0x110
[<ffffffff810f8889>] load_module+0x16a9/0x1b30
[<ffffffff810f3d30>] ? store_uevent+0x70/0x70
[<ffffffff810f49b9>] ? copy_module_from_fd.isra.44+0x129/0x180
[<ffffffff810f8ec6>] SyS_finit_module+0xa6/0xd0
[<ffffffff8166ce29>] system_call_fastpath+0x12/0x17
Code: c4 08 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 c7 c7 20 42 8a 81 31 c0 e8 fc
80 5e 00 eb 80 48 c7 c7 78 42 8a 81 31 c0 e8 ec 80 5e 00 <0f> 0b 66 66 66 66 2e
0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55
RIP [<ffffffff81076b81>] warn_slowpath_common+0xc1/0xd0
RSP <ffff8807fc5afc68>
---[ end trace 428218934a12088b ]---
Successfully tested by me.
Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org>
Cc: Fabian Frederick <fabf-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Cc: vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org
Cc: linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[v2]: add /proc/sys/kernel/bug_on_warn, additional documentation, modify
!slowpath cases
---
Documentation/kdump/kdump.txt | 7 +++++++
Documentation/kernel-parameters.txt | 3 +++
Documentation/sysctl/kernel.txt | 12 ++++++++++++
include/asm-generic/bug.h | 12 ++++++++++--
include/linux/kernel.h | 1 +
include/uapi/linux/sysctl.h | 1 +
kernel/panic.c | 21 ++++++++++++++++++++-
kernel/sysctl.c | 7 +++++++
kernel/sysctl_binary.c | 1 +
9 files changed, 62 insertions(+), 3 deletions(-)
diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
index 6c0b9f2..a04ed72 100644
--- a/Documentation/kdump/kdump.txt
+++ b/Documentation/kdump/kdump.txt
@@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
http://people.redhat.com/~anderson/
+Trigger Kdump on WARN()
+=======================
+
+The kernel parameter, bug_on_warn, calls BUG() in all WARN() paths. This
+will cause a kdump to occur at the BUG() call. In cases where a user
+wants to specify this during runtime, /proc/sys/kernel/bug_on_warn can be
+set to 1 to achieve the same behaviour.
Contact
=======
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 988160a..3890a3a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -553,6 +553,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
bttv.pll= See Documentation/video4linux/bttv/Insmod-options
bttv.tuner=
+ bug_on_warn BUG() instead of WARN(). Useful to cause kdump
+ on a WARN().
+
bulk_remove=off [PPC] This parameter disables the use of the pSeries
firmware feature for flushing multiple hpte entries
at a time.
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 57baff5..dcadcdc 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -23,6 +23,7 @@ show up in /proc/sys/kernel:
- auto_msgmni
- bootloader_type [ X86 only ]
- bootloader_version [ X86 only ]
+- bug_on_warn
- callhome [ S390 only ]
- cap_last_cap
- core_pattern
@@ -152,6 +153,17 @@ Documentation/x86/boot.txt for additional information.
==============================================================
+bug_on_warn:
+
+Calls BUG() in the WARN() path when set to 1. This is useful to avoid
+a kernel rebuild when attempting to kdump at the location of a WARN().
+
+0: only WARN(), default behaviour.
+
+1: call BUG() after printing out WARN() location.
+
+==============================================================
+
callhome:
Controls the kernel's callhome behavior in case of a kernel panic.
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 630dd23..4d0c763 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -75,10 +75,18 @@ extern void warn_slowpath_null(const char *file, const int line);
#define __WARN_printf_taint(taint, arg...) \
warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
#else
-#define __WARN() __WARN_TAINT(TAINT_WARN)
+#define check_bug_on_warn() \
+ do { \
+ if (bug_on_warn) \
+ BUG(); \
+ } while (0)
+
+#define __WARN() \
+ do { __WARN_TAINT(TAINT_WARN); check_bug_on_warn(); } while (0)
+
#define __WARN_printf(arg...) do { printk(arg); __WARN(); } while (0)
#define __WARN_printf_taint(taint, arg...) \
- do { printk(arg); __WARN_TAINT(taint); } while (0)
+ do { printk(arg); __WARN_TAINT(taint); check_bug_on_warn(); } while (0)
#endif
#ifndef WARN_ON
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 40728cf..4094a60 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -422,6 +422,7 @@ extern int panic_on_oops;
extern int panic_on_unrecovered_nmi;
extern int panic_on_io_nmi;
extern int sysctl_panic_on_stackoverflow;
+extern int bug_on_warn;
/*
* Only to be used by arch init code. If the user over-wrote the default
* CONFIG_PANIC_TIMEOUT, honor it.
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 43aaba1..2ba0a58 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -153,6 +153,7 @@ enum
KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+ KERN_BUG_ON_WARN=77, /* int: call BUG() in WARN() functions */
};
diff --git a/kernel/panic.c b/kernel/panic.c
index d09dc5c..a6d2e2f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -33,6 +33,7 @@ static int pause_on_oops;
static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
static bool crash_kexec_post_notifiers;
+int bug_on_warn;
int panic_timeout = CONFIG_PANIC_TIMEOUT;
EXPORT_SYMBOL_GPL(panic_timeout);
@@ -420,13 +421,24 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
{
disable_trace_on_warning();
- pr_warn("------------[ cut here ]------------\n");
+ if (!bug_on_warn)
+ pr_warn("------------[ cut here ]------------\n");
pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
raw_smp_processor_id(), current->pid, file, line, caller);
if (args)
vprintk(args->fmt, args->args);
+ if (bug_on_warn) {
+ pr_warn("bug_on_warn set, calling BUG()...\n");
+ /*
+ * A flood of WARN()s may occur. Prevent further WARN()s
+ * from panicking the system.
+ */
+ bug_on_warn = 0;
+ BUG();
+ }
+
print_modules();
dump_stack();
print_oops_end_marker();
@@ -501,3 +513,10 @@ static int __init oops_setup(char *s)
return 0;
}
early_param("oops", oops_setup);
+
+static int __init bug_on_warn_setup(char *s)
+{
+ bug_on_warn = 1;
+ return 0;
+}
+early_param("bug_on_warn", bug_on_warn_setup);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4aada6d..030bb5d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1103,6 +1103,13 @@ static struct ctl_table kern_table[] = {
.proc_handler = proc_dointvec,
},
#endif
+ {
+ .procname = "bug_on_warn",
+ .data = &bug_on_warn,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
{ }
};
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 9a4f750..28376bf 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
{ CTL_INT, KERN_COMPAT_LOG, "compat-log" },
{ CTL_INT, KERN_MAX_LOCK_DEPTH, "max_lock_depth" },
{ CTL_INT, KERN_PANIC_ON_NMI, "panic_on_unrecovered_nmi" },
+ { CTL_INT, KERN_BUG_ON_WARN, "bug_on_warn" },
{}
};
--
1.7.9.3
^ permalink raw reply related
* Re: [PATCH 2/2] fs: Support compiling out sendfile
From: Eric Paris @ 2014-10-21 17:20 UTC (permalink / raw)
To: josh
Cc: H. Peter Anvin, Pieter Smith, Alexander Viro, Andrew Morton,
Matt Turner, Michal Hocko, Paul E. McKenney, Fabian Frederick,
Tejun Heo, 蔡正龙, Luis R. Rodriguez,
Peter Foley, Konstantin Khlebnikov, Eric W. Biederman,
Oleg Nesterov, Andy Lutomirski, David Herrmann, Kees Cook,
linux-fsdevel, open list, ABI/API
In-Reply-To: <20141021171814.GA14704@cloud>
On Tue, 2014-10-21 at 10:18 -0700, josh@joshtriplett.org wrote:
> On Tue, Oct 21, 2014 at 08:37:00AM -0700, H. Peter Anvin wrote:
> > On 10/20/2014 02:48 PM, Pieter Smith wrote:
> > > Many embedded systems will not need this syscall, and omitting it
> > > saves space. Add a new EXPERT config option CONFIG_SENDFILE_SYSCALL
> > > (default y) to support compiling it out.
> >
> > <bikeshed>
> > I believe these options ought to be CONFIG_SYSCALL_*
> > </bikeshed>
>
> I agree. I think people started using CONFIG_*_SYSCALL because of
> things like AUDITSYSCALL
AUDITSYSCALL audits syscalls. It doesn't actually implement any
syscalls. You are right about SYSFS_SYSCALL though...
^ 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