* Re: [PATCH 11/19] block: implement bio helper to add iter bvec pages to bio
From: Christoph Hellwig @ 2019-03-08 7:55 UTC (permalink / raw)
To: Jens Axboe
Cc: Ming Lei, Ming Lei, Eric Biggers, open list:AIO, linux-block,
linux-api, Christoph Hellwig, Jeff Moyer, Avi Kivity, jannh,
Al Viro
In-Reply-To: <b54683e8-fbff-01ab-db98-c35de8e00e9f@kernel.dk>
On Tue, Feb 26, 2019 at 07:28:54PM -0700, Jens Axboe wrote:
> On 2/26/19 7:21 PM, Ming Lei wrote:
> > On Tue, Feb 26, 2019 at 06:57:16PM -0700, Jens Axboe wrote:
> >> On 2/26/19 6:53 PM, Ming Lei wrote:
> >>> On Tue, Feb 26, 2019 at 06:47:54PM -0700, Jens Axboe wrote:
> >>>> On 2/26/19 6:21 PM, Ming Lei wrote:
> >>>>> On Tue, Feb 26, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>>
> >>>>>> On 2/25/19 9:34 PM, Jens Axboe wrote:
> >>>>>>> On 2/25/19 8:46 PM, Eric Biggers wrote:
> >>>>>>>> Hi Jens,
> >>>>>>>>
> >>>>>>>> On Thu, Feb 21, 2019 at 10:45:27AM -0700, Jens Axboe wrote:
> >>>>>>>>> On 2/20/19 3:58 PM, Ming Lei wrote:
> >>>>>>>>>> On Mon, Feb 11, 2019 at 12:00:41PM -0700, Jens Axboe wrote:
> >>>>>>>>>>> For an ITER_BVEC, we can just iterate the iov and add the pages
> >>>>>>>>>>> to the bio directly. This requires that the caller doesn't releases
> >>>>>>>>>>> the pages on IO completion, we add a BIO_NO_PAGE_REF flag for that.
> >>>>>>>>>>>
> >>>>>>>>>>> The current two callers of bio_iov_iter_get_pages() are updated to
> >>>>>>>>>>> check if they need to release pages on completion. This makes them
> >>>>>>>>>>> work with bvecs that contain kernel mapped pages already.
> >>>>>>>>>>>
> >>>>>>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.com>
> >>>>>>>>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >>>>>>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>>>>>>>>>> ---
> >>>>>>>>>>> block/bio.c | 59 ++++++++++++++++++++++++++++++++-------
> >>>>>>>>>>> fs/block_dev.c | 5 ++--
> >>>>>>>>>>> fs/iomap.c | 5 ++--
> >>>>>>>>>>> include/linux/blk_types.h | 1 +
> >>>>>>>>>>> 4 files changed, 56 insertions(+), 14 deletions(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/block/bio.c b/block/bio.c
> >>>>>>>>>>> index 4db1008309ed..330df572cfb8 100644
> >>>>>>>>>>> --- a/block/bio.c
> >>>>>>>>>>> +++ b/block/bio.c
> >>>>>>>>>>> @@ -828,6 +828,23 @@ int bio_add_page(struct bio *bio, struct page *page,
> >>>>>>>>>>> }
> >>>>>>>>>>> EXPORT_SYMBOL(bio_add_page);
> >>>>>>>>>>>
> >>>>>>>>>>> +static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter)
> >>>>>>>>>>> +{
> >>>>>>>>>>> + const struct bio_vec *bv = iter->bvec;
> >>>>>>>>>>> + unsigned int len;
> >>>>>>>>>>> + size_t size;
> >>>>>>>>>>> +
> >>>>>>>>>>> + len = min_t(size_t, bv->bv_len, iter->count);
> >>>>>>>>>>> + size = bio_add_page(bio, bv->bv_page, len,
> >>>>>>>>>>> + bv->bv_offset + iter->iov_offset);
> >>>>>>>>>>
> >>>>>>>>>> iter->iov_offset needs to be subtracted from 'len', looks
> >>>>>>>>>> the following delta change[1] is required, otherwise memory corruption
> >>>>>>>>>> can be observed when running xfstests over loop/dio.
> >>>>>>>>>
> >>>>>>>>> Thanks, I folded this in.
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Jens Axboe
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> syzkaller started hitting a crash on linux-next starting with this commit, and
> >>>>>>>> it still occurs even with your latest version that has Ming's fix folded in.
> >>>>>>>> Specifically, commit a566653ab5ab80a from your io_uring branch with commit date
> >>>>>>>> Sun Feb 24 08:20:53 2019 -0700.
> >>>>>>>>
> >>>>>>>> Reproducer:
> >>>>>>>>
> >>>>>>>> #define _GNU_SOURCE
> >>>>>>>> #include <fcntl.h>
> >>>>>>>> #include <linux/loop.h>
> >>>>>>>> #include <sys/ioctl.h>
> >>>>>>>> #include <sys/sendfile.h>
> >>>>>>>> #include <sys/syscall.h>
> >>>>>>>> #include <unistd.h>
> >>>>>>>>
> >>>>>>>> int main(void)
> >>>>>>>> {
> >>>>>>>> int memfd, loopfd;
> >>>>>>>>
> >>>>>>>> memfd = syscall(__NR_memfd_create, "foo", 0);
> >>>>>>>>
> >>>>>>>> pwrite(memfd, "\xa8", 1, 4096);
> >>>>>>>>
> >>>>>>>> loopfd = open("/dev/loop0", O_RDWR|O_DIRECT);
> >>>>>>>>
> >>>>>>>> ioctl(loopfd, LOOP_SET_FD, memfd);
> >>>>>>>>
> >>>>>>>> sendfile(loopfd, loopfd, NULL, 1000000);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Crash:
> >>>>>>>>
> >>>>>>>> page:ffffea0001a6aab8 count:0 mapcount:0 mapping:0000000000000000 index:0x0
> >>>>>>>> flags: 0x100000000000000()
> >>>>>>>> raw: 0100000000000000 ffffea0001ad2c50 ffff88807fca49d0 0000000000000000
> >>>>>>>> raw: 0000000000000000 0000000000000000 00000000ffffffff
> >>>>>>>> page dumped because: VM_BUG_ON_PAGE(page_ref_count(page) == 0)
> >>>>>>>
> >>>>>>> I see what this is, I'll cut a fix for this tomorrow.
> >>>>>>
> >>>>>> Folded in a fix for this, it's in my current io_uring branch and my for-next
> >>>>>> branch.
> >>>>>
> >>>>> Hi Jens,
> >>>>>
> >>>>> I saw the following change is added:
> >>>>>
> >>>>> + if (size == len) {
> >>>>> + /*
> >>>>> + * For the normal O_DIRECT case, we could skip grabbing this
> >>>>> + * reference and then not have to put them again when IO
> >>>>> + * completes. But this breaks some in-kernel users, like
> >>>>> + * splicing to/from a loop device, where we release the pipe
> >>>>> + * pages unconditionally. If we can fix that case, we can
> >>>>> + * get rid of the get here and the need to call
> >>>>> + * bio_release_pages() at IO completion time.
> >>>>> + */
> >>>>> + get_page(bv->bv_page);
> >>>>>
> >>>>> Now the 'bv' may point to more than one page, so the following one may be
> >>>>> needed:
> >>>>>
> >>>>> int i;
> >>>>> struct bvec_iter_all iter_all;
> >>>>> struct bio_vec *tmp;
> >>>>>
> >>>>> mp_bvec_for_each_segment(tmp, bv, i, iter_all)
> >>>>> get_page(tmp->bv_page);
> >>>>
> > Some following optimization can be done, such as removing
> > biovec_phys_mergeable() from blk_bio_segment_split().
>
> I think we really need a fast path for <= PAGE_SIZE IOs, to the extent
> that it is possible. But iteration startup cost is a problem in a lot of
> spots, and a split fast path will only help a bit for that specific
> case.
FYI, I've got a nice fast path for the driver side in nvme here, but
I'll need to do some more testing before submitting it:
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-optimize-single-segment-io
But in the block layer I think one major issue is all the phys_segments
crap. What we really should do is to remove bi_phys_segments and all
the front/back segment crap and only do the calculation of the actual
per-bio segments once, just before adding the bio to the segment.
And don't bother with it at all unless the driver has weird segment
size or boundary limitations.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCHv6 07/10] acpi/hmat: Register processor domain to its memory
From: Keith Busch @ 2019-03-07 15:19 UTC (permalink / raw)
To: Brice Goglin
Cc: Busch, Keith, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org, linux-mm@kvack.org,
linux-api@vger.kernel.org, Greg Kroah-Hartman, Rafael Wysocki,
Hansen, Dave, Williams, Dan J
In-Reply-To: <8fb27d2c-2165-7029-6ea1-94fc379b3be7@inria.fr>
Hi Brice,
Please see v7 of this series from last week instead for reviews:
https://patchwork.kernel.org/cover/10832365/
^ permalink raw reply
* Re: [PATCHv6 07/10] acpi/hmat: Register processor domain to its memory
From: Brice Goglin @ 2019-03-07 11:49 UTC (permalink / raw)
To: Keith Busch, linux-kernel, linux-acpi, linux-mm, linux-api
Cc: Greg Kroah-Hartman, Rafael Wysocki, Dave Hansen, Dan Williams
In-Reply-To: <20190214171017.9362-8-keith.busch@intel.com>
Le 14/02/2019 à 18:10, Keith Busch a écrit :
> If the HMAT Subsystem Address Range provides a valid processor proximity
> domain for a memory domain, or a processor domain matches the performance
> access of the valid processor proximity domain, register the memory
> target with that initiator so this relationship will be visible under
> the node's sysfs directory.
>
> By registering only the best performing relationships, this provides the
> most useful information applications may want to know when considering
> which CPU they should run on for a given memory node, or which memory
> node they should allocate memory from for a given CPU.
>
> Since HMAT requires valid address ranges have an equivalent SRAT entry,
> verify each memory target satisfies this requirement.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
[...]
> +static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
> +{
> + struct memory_initiator *intitator;
> +
> + list_for_each_entry(intitator, &initiators, node)
> + if (intitator->processor_pxm == cpu_pxm)
> + return intitator;
> + return NULL;
> +}
Typo intitator -> initiator
> +static __init void alloc_memory_initiator(unsigned int cpu_pxm)
> +{
> + struct memory_initiator *intitator;
> +
> + if (pxm_to_node(cpu_pxm) == NUMA_NO_NODE)
> + return;
> +
> + intitator = find_mem_initiator(cpu_pxm);
> + if (intitator)
> + return;
> +
> + intitator = kzalloc(sizeof(*intitator), GFP_KERNEL);
> + if (!intitator)
> + return;
> +
> + intitator->processor_pxm = cpu_pxm;
> + list_add_tail(&intitator->node, &initiators);
> +}
Same typo
> +static __init void hmat_register_target_initiators(struct memory_target *target)
> +{
> + static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
> + struct memory_initiator *initiator;
> + unsigned int mem_nid, cpu_nid;
> + struct memory_locality *loc = NULL;
> + u32 best = 0;
> + int i;
> +
> + if (target->processor_pxm == PXM_INVAL)
> + return;
This test above looks wrong to me. First, it means that either you
return from here, or from the next branch below, hence the loop that
looks for best performance is dead code. Secondly, it means that memory
targets without a PXM never get an initiator.
I verified that removing this test makes things look better on my HMAT
tests.
> + mem_nid = pxm_to_node(target->memory_pxm);
> +
> + /*
> + * If the Address Range Structure provides a local processor pxm, link
> + * only that one. Otherwise, find the best performance attribtes and
> + * register all initiators that match.
> + */
> + if (target->processor_pxm != PXM_INVAL) {
> + cpu_nid = pxm_to_node(target->processor_pxm);
> + register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
> + return;
> + }
This seems to contradict your first paragraph in the header where you say
"or a processor domain matches the performance access of the valid processor proximity domain".
By returning here, you're only keeping the the local PXM and ignoring
other initiators that may have the same performance.
I am testing a HMAT where one memory target has same performance to two
processor proxdomains. If no processor proxdomain is set in the HMAT for
this target, I get two initiators as expected. If proxdomain is
explicitly set in the HMAT, I get only that one as initiator.
Brice
^ permalink raw reply
* Re: [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Jiri Kosina @ 2019-03-07 5:46 UTC (permalink / raw)
To: Dominique Martinet
Cc: Andrew Morton, Vlastimil Babka, Linus Torvalds, linux-kernel,
linux-mm, linux-api, Peter Zijlstra, Greg KH, Jann Horn,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss
In-Reply-To: <20190307004036.GA16785@nautica>
On Thu, 7 Mar 2019, Dominique Martinet wrote:
> > >
> > > addr = 4095
> > > vma->vm_end = 4096
> > > pages = 1000
> > >
> > > then `end' is 4096 and `(end - addr) << PAGE_SHIFT' is zero, but it
> > > should have been 1.
> >
> > Good catch! It should rather be something like
> >
> > unsigned long pages = (end >> PAGE_SHIFT) - (addr >> PAGE_SHIFT);
>
> That would be 0 for addr = 0 and vma->vm_end = 1; I assume we would
> still want to count that as one page.
Yeah, that was bogus as well, ETOOTIRED yesterday, sorry for the noise.
Both the variants are off.
> I'm not too familiar with this area of the code, but I think there's a
> handy macro we can use for this, perhaps
> DIV_ROUND_UP(end - addr, PAGE_SIZE) ?
>
> kernel/kexec_core.c has defined PAGE_COUNT() which seems more
> appropriate but I do not see a global equivalent
> #define PAGE_COUNT(x) (((x) + PAGE_SIZE - 1) >> PAGE_SHIFT)
I'll fix that up when doing the other changes requested by Andrew.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Dominique Martinet @ 2019-03-07 0:40 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Vlastimil Babka, Linus Torvalds, linux-kernel,
linux-mm, linux-api, Peter Zijlstra, Greg KH, Jann Horn,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss
In-Reply-To: <nycvar.YFH.7.76.1903070047360.19912@cbobk.fhfr.pm>
Jiri Kosina wrote on Thu, Mar 07, 2019:
> > I'm not sure this is correct in all cases. If
> >
> > addr = 4095
> > vma->vm_end = 4096
> > pages = 1000
> >
> > then `end' is 4096 and `(end - addr) << PAGE_SHIFT' is zero, but it
> > should have been 1.
>
> Good catch! It should rather be something like
>
> unsigned long pages = (end >> PAGE_SHIFT) - (addr >> PAGE_SHIFT);
That would be 0 for addr = 0 and vma->vm_end = 1; I assume we would
still want to count that as one page.
I'm not too familiar with this area of the code, but I think there's a
handy macro we can use for this, perhaps
DIV_ROUND_UP(end - addr, PAGE_SIZE) ?
kernel/kexec_core.c has defined PAGE_COUNT() which seems more
appropriate but I do not see a global equivalent
#define PAGE_COUNT(x) (((x) + PAGE_SIZE - 1) >> PAGE_SHIFT)
--
Dominique
^ permalink raw reply
* Re: [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Jiri Kosina @ 2019-03-07 0:01 UTC (permalink / raw)
To: Andrew Morton
Cc: Vlastimil Babka, Linus Torvalds, linux-kernel, linux-mm,
linux-api, Peter Zijlstra, Greg KH, Jann Horn, Dominique Martinet,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss
In-Reply-To: <20190306151351.f8ae1acae51ccad1a3537284@linux-foundation.org>
On Wed, 6 Mar 2019, Andrew Morton wrote:
> > The semantics of what mincore() considers to be resident is not completely
> > clear, but Linux has always (since 2.3.52, which is when mincore() was
> > initially done) treated it as "page is available in page cache".
> >
> > That's potentially a problem, as that [in]directly exposes meta-information
> > about pagecache / memory mapping state even about memory not strictly belonging
> > to the process executing the syscall, opening possibilities for sidechannel
> > attacks.
> >
> > Change the semantics of mincore() so that it only reveals pagecache information
> > for non-anonymous mappings that belog to files that the calling process could
> > (if it tried to) successfully open for writing.
>
> "for writing" comes as a bit of a surprise. Why not for reading?
I guess this is a rhetorical question from you :) but fair enough, good
point, I'll explain this a bit more in the changelog and in the code
comments.
> > @@ -189,8 +197,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
> > vma = find_vma(current->mm, addr);
> > if (!vma || addr < vma->vm_start)
> > return -ENOMEM;
> > - mincore_walk.mm = vma->vm_mm;
> > end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
> > + if (!can_do_mincore(vma)) {
> > + unsigned long pages = (end - addr) >> PAGE_SHIFT;
>
> I'm not sure this is correct in all cases. If
>
> addr = 4095
> vma->vm_end = 4096
> pages = 1000
>
> then `end' is 4096 and `(end - addr) << PAGE_SHIFT' is zero, but it
> should have been 1.
Good catch! It should rather be something like
unsigned long pages = (end >> PAGE_SHIFT) - (addr >> PAGE_SHIFT);
I'll fix that up and resend tomorrow.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Andrew Morton @ 2019-03-06 23:38 UTC (permalink / raw)
To: Dominique Martinet
Cc: Jiri Kosina, Vlastimil Babka, Linus Torvalds, linux-kernel,
linux-mm, linux-api, Peter Zijlstra, Greg KH, Jann Horn,
Andy Lutomirski, Cyril Hrubis, Daniel Gruss, Dave Chinner,
Kevin Easton, Kirill A. Shutemov, Matthew Wilcox, Tejun Heo
In-Reply-To: <20190306233209.GA7753@nautica>
On Thu, 7 Mar 2019 00:32:09 +0100 Dominique Martinet <asmadeus@codewreck.org> wrote:
> Andrew Morton wrote on Wed, Mar 06, 2019:
> > On Wed, 6 Mar 2019 23:48:03 +0100 (CET) Jiri Kosina <jikos@kernel.org> wrote:
> >
> > > 3/3 is actually waiting for your decision, see
> > >
> > > https://lore.kernel.org/lkml/20190212063643.GL15609@dhcp22.suse.cz/
> >
> > I pity anyone who tried to understand this code by reading this code.
> > Can we please get some careful commentary in there explaining what is
> > going on, and why things are thus?
> >
> > I guess the [3/3] change makes sense, although it's unclear whether
> > anyone really needs it? 5.0 was released with 574823bfab8 ("Change
> > mincore() to count "mapped" pages rather than "cached" pages") so we'll
> > have a release cycle to somewhat determine how much impact 574823bfab8
> > has on users. How about I queue up [3/3] and we reevaluate its
> > desirability in a couple of months?
>
> FWIW,
>
> 574823bfab8 has been reverted in 30bac164aca750, included in 5.0-rc4, so
> the controversial change has only been there from 5.0-rc1 to 5.0-rc3
Ah, OK, thanks, I misread.
Linus, do you have thoughts on
http://lkml.kernel.org/r/20190130124420.1834-4-vbabka@suse.cz ?
^ permalink raw reply
* Re: [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Dominique Martinet @ 2019-03-06 23:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Jiri Kosina, Vlastimil Babka, Linus Torvalds, linux-kernel,
linux-mm, linux-api, Peter Zijlstra, Greg KH, Jann Horn,
Andy Lutomirski, Cyril Hrubis, Daniel Gruss, Dave Chinner,
Kevin Easton, Kirill A. Shutemov, Matthew Wilcox, Tejun Heo
In-Reply-To: <20190306152337.e06cbc530fbfbcfcfe0dc37c@linux-foundation.org>
Andrew Morton wrote on Wed, Mar 06, 2019:
> On Wed, 6 Mar 2019 23:48:03 +0100 (CET) Jiri Kosina <jikos@kernel.org> wrote:
>
> > 3/3 is actually waiting for your decision, see
> >
> > https://lore.kernel.org/lkml/20190212063643.GL15609@dhcp22.suse.cz/
>
> I pity anyone who tried to understand this code by reading this code.
> Can we please get some careful commentary in there explaining what is
> going on, and why things are thus?
>
> I guess the [3/3] change makes sense, although it's unclear whether
> anyone really needs it? 5.0 was released with 574823bfab8 ("Change
> mincore() to count "mapped" pages rather than "cached" pages") so we'll
> have a release cycle to somewhat determine how much impact 574823bfab8
> has on users. How about I queue up [3/3] and we reevaluate its
> desirability in a couple of months?
FWIW,
574823bfab8 has been reverted in 30bac164aca750, included in 5.0-rc4, so
the controversial change has only been there from 5.0-rc1 to 5.0-rc3
--
Dominique
^ permalink raw reply
* Re: [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Andrew Morton @ 2019-03-06 23:23 UTC (permalink / raw)
To: Jiri Kosina
Cc: Vlastimil Babka, Linus Torvalds, linux-kernel, linux-mm,
linux-api, Peter Zijlstra, Greg KH, Jann Horn, Andy Lutomirski,
Cyril Hrubis, Daniel Gruss, Dave Chinner, Dominique Martinet,
Kevin Easton, Kirill A. Shutemov, Matthew Wilcox, Tejun Heo
In-Reply-To: <nycvar.YFH.7.76.1903062342020.19912@cbobk.fhfr.pm>
On Wed, 6 Mar 2019 23:48:03 +0100 (CET) Jiri Kosina <jikos@kernel.org> wrote:
> 3/3 is actually waiting for your decision, see
>
> https://lore.kernel.org/lkml/20190212063643.GL15609@dhcp22.suse.cz/
I pity anyone who tried to understand this code by reading this code.
Can we please get some careful commentary in there explaining what is
going on, and why things are thus?
I guess the [3/3] change makes sense, although it's unclear whether
anyone really needs it? 5.0 was released with 574823bfab8 ("Change
mincore() to count "mapped" pages rather than "cached" pages") so we'll
have a release cycle to somewhat determine how much impact 574823bfab8
has on users. How about I queue up [3/3] and we reevaluate its
desirability in a couple of months?
^ permalink raw reply
* Re: [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Andrew Morton @ 2019-03-06 23:13 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Linus Torvalds, linux-kernel, linux-mm, linux-api, Peter Zijlstra,
Greg KH, Jann Horn, Jiri Kosina, Dominique Martinet,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss,
Jiri Kosina
In-Reply-To: <20190130124420.1834-2-vbabka@suse.cz>
On Wed, 30 Jan 2019 13:44:18 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:
> From: Jiri Kosina <jkosina@suse.cz>
>
> The semantics of what mincore() considers to be resident is not completely
> clear, but Linux has always (since 2.3.52, which is when mincore() was
> initially done) treated it as "page is available in page cache".
>
> That's potentially a problem, as that [in]directly exposes meta-information
> about pagecache / memory mapping state even about memory not strictly belonging
> to the process executing the syscall, opening possibilities for sidechannel
> attacks.
>
> Change the semantics of mincore() so that it only reveals pagecache information
> for non-anonymous mappings that belog to files that the calling process could
> (if it tried to) successfully open for writing.
"for writing" comes as a bit of a surprise. Why not for reading?
Could we please explain the reasoning in the changelog and in the
(presently absent) comments which describe can_do_mincore()?
> @@ -189,8 +197,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
> vma = find_vma(current->mm, addr);
> if (!vma || addr < vma->vm_start)
> return -ENOMEM;
> - mincore_walk.mm = vma->vm_mm;
> end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
> + if (!can_do_mincore(vma)) {
> + unsigned long pages = (end - addr) >> PAGE_SHIFT;
I'm not sure this is correct in all cases. If
addr = 4095
vma->vm_end = 4096
pages = 1000
then `end' is 4096 and `(end - addr) << PAGE_SHIFT' is zero, but it
should have been 1.
Please check?
A mincore test suite in tools/testing/selftests would be useful,
methinks. To exercise such corner cases, check for future breakage,
etc.
> + memset(vec, 1, pages);
> + return pages;
> + }
> + mincore_walk.mm = vma->vm_mm;
> err = walk_page_range(addr, end, &mincore_walk);
> if (err < 0)
> return err;
^ permalink raw reply
* Re: [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Jiri Kosina @ 2019-03-06 22:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Vlastimil Babka, Linus Torvalds, linux-kernel, linux-mm,
linux-api, Peter Zijlstra, Greg KH, Jann Horn, Andy Lutomirski,
Cyril Hrubis, Daniel Gruss, Dave Chinner, Dominique Martinet,
Kevin Easton, Kirill A. Shutemov, Matthew Wilcox, Tejun Heo
In-Reply-To: <20190306143547.c686225447822beaf3b6e139@linux-foundation.org>
On Wed, 6 Mar 2019, Andrew Morton wrote:
> > could you please take at least the correct and straightforward fix for
> > mincore() before we figure out how to deal with the slightly less
> > practical RWF_NOWAIT? Thanks.
>
> I assume we're talking about [1/3] and [2/3] from this thread?
>
> Can we have a resend please? Gather the various acks and revisions,
> make changelog changes to address the review questions and comments?
1/3 is clearly the one to be merged. The version with all the acks
gathered is in this thread, at
https://lore.kernel.org/lkml/de52b3bd-4e39-c133-542a-0a9c5e357404@suse.cz/
Attaching the patch also at the end of this mail so that it could be
easily picked up.
I am unfortunately not sure what changelog changes you are talking about,
there were none requested during the review as far as I know.
2/3 is clearly postponed for now, it needs more thinking.
3/3 is actually waiting for your decision, see
https://lore.kernel.org/lkml/20190212063643.GL15609@dhcp22.suse.cz/
The 1/3 patch to be merged in any case:
=== cut here ===
From: Jiri Kosina <jkosina@suse.cz>
Date: Wed, 16 Jan 2019 20:53:17 +0100
Subject: [PATCH v2] mm/mincore: make mincore() more conservative
The semantics of what mincore() considers to be resident is not completely
clear, but Linux has always (since 2.3.52, which is when mincore() was
initially done) treated it as "page is available in page cache".
That's potentially a problem, as that [in]directly exposes meta-information
about pagecache / memory mapping state even about memory not strictly belonging
to the process executing the syscall, opening possibilities for sidechannel
attacks.
Change the semantics of mincore() so that it only reveals pagecache information
for non-anonymous mappings that belog to files that the calling process could
(if it tried to) successfully open for writing.
[mhocko@suse.com: restructure can_do_mincore() conditions]
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Originally-by: Dominique Martinet <asmadeus@codewreck.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Josh Snyder <joshs@netflix.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
mm/mincore.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..b8842b849604 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,16 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
return 0;
}
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ if (vma_is_anonymous(vma))
+ return true;
+ if (!vma->vm_file)
+ return false;
+ return inode_owner_or_capable(file_inode(vma->vm_file)) ||
+ inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
+}
+
/*
* Do a chunk of "sys_mincore()". We've already checked
* all the arguments, we hold the mmap semaphore: we should
@@ -189,8 +199,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
- mincore_walk.mm = vma->vm_mm;
end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
+ if (!can_do_mincore(vma)) {
+ unsigned long pages = (end - addr) >> PAGE_SHIFT;
+ memset(vec, 1, pages);
+ return pages;
+ }
+ mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
return err;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* Re: [PATCH 0/3] mincore() and IOCB_NOWAIT adjustments
From: Andrew Morton @ 2019-03-06 22:35 UTC (permalink / raw)
To: Jiri Kosina
Cc: Vlastimil Babka, Linus Torvalds, linux-kernel, linux-mm,
linux-api, Peter Zijlstra, Greg KH, Jann Horn, Andy Lutomirski,
Cyril Hrubis, Daniel Gruss, Dave Chinner, Dominique Martinet,
Kevin Easton, Kirill A. Shutemov, Matthew Wilcox, Tejun Heo
In-Reply-To: <nycvar.YFH.7.76.1903061310170.19912@cbobk.fhfr.pm>
On Wed, 6 Mar 2019 13:11:39 +0100 (CET) Jiri Kosina <jikos@kernel.org> wrote:
> On Wed, 30 Jan 2019, Vlastimil Babka wrote:
>
> > I've collected the patches from the discussion for formal posting. The first
> > two should be settled already, third one is the possible improvement I've
> > mentioned earlier, where only in restricted case we resort to existence of page
> > table mapping (the original and later reverted approach from Linus) instead of
> > faking the result completely. Review and testing welcome.
> >
> > The consensus seems to be going through -mm tree for 5.1, unless Linus wants
> > them alredy for 5.0.
> >
> > Jiri Kosina (2):
> > mm/mincore: make mincore() more conservative
> > mm/filemap: initiate readahead even if IOCB_NOWAIT is set for the I/O
> >
> > Vlastimil Babka (1):
> > mm/mincore: provide mapped status when cached status is not allowed
>
> Andrew,
>
> could you please take at least the correct and straightforward fix for
> mincore() before we figure out how to deal with the slightly less
> practical RWF_NOWAIT? Thanks.
I assume we're talking about [1/3] and [2/3] from this thread?
Can we have a resend please? Gather the various acks and revisions,
make changelog changes to address the review questions and comments?
Thanks.
^ permalink raw reply
* [PATCH RESEND v5 5/5] binfmt_*: scope path resolution of interpreters
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Christian Brauner,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
containers, linux-fsdevel, linux-api, linux-kernel, linux-arch
In-Reply-To: <20190306191244.8691-1-cyphar@cyphar.com>
The need to be able to scope path resolution of interpreters became
clear with one of the possible vectors used in CVE-2019-5736 (which
most major container runtimes were vulnerable to).
Naively, it might seem that openat(2) -- which supports path scoping --
can be combined with execveat(AT_EMPTY_PATH) to trivially scope the
binary being executed. Unfortunately, a "bad binary" (usually a symlink)
could be written as a #!-style script with the symlink target as the
interpreter -- which would be completely missed by just scoping the
openat(2). An example of this being exploitable is CVE-2019-5736.
In order to get around this, we need to pass down to each binfmt_*
implementation the scoping flags requested in execveat(2). In order to
maintain backwards-compatibility we only pass the scoping AT_* flags.
To avoid breaking userspace (in the exceptionally rare cases where you
have #!-scripts with a relative path being execveat(2)-ed with dfd !=
AT_FDCWD), we only pass dfd down to binfmt_* if any of our new flags are
set in execveat(2).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/binfmt_em86.c | 4 ++--
fs/binfmt_misc.c | 2 +-
fs/binfmt_script.c | 2 +-
fs/exec.c | 26 ++++++++++++++++++++++----
include/linux/binfmts.h | 1 +
include/linux/fs.h | 9 +++++++--
include/uapi/linux/fcntl.h | 6 ++++++
9 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 54207327f98f..eef86ffa38c8 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -777,7 +777,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
if (elf_interpreter[elf_ppnt->p_filesz - 1] != '\0')
goto out_free_interp;
- interpreter = open_exec(elf_interpreter);
+ interpreter = openat_exec(bprm->dfd, elf_interpreter, bprm->flags);
retval = PTR_ERR(interpreter);
if (IS_ERR(interpreter))
goto out_free_interp;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index b53bb3729ac1..c463c6428f77 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -263,7 +263,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
kdebug("Using ELF interpreter %s", interpreter_name);
/* replace the program with the interpreter */
- interpreter = open_exec(interpreter_name);
+ interpreter = openat_exec(bprm->dfd, interpreter_name, bprm->flags);
retval = PTR_ERR(interpreter);
if (IS_ERR(interpreter)) {
interpreter = NULL;
diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index dd2d3f0cd55d..3ee46b0dc0d4 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -81,10 +81,10 @@ static int load_em86(struct linux_binprm *bprm)
/*
* OK, now restart the process with the interpreter's inode.
- * Note that we use open_exec() as the name is now in kernel
+ * Note that we use openat_exec() as the name is now in kernel
* space, and we don't need to copy it.
*/
- file = open_exec(interp);
+ file = openat_exec(binprm->dfd, interp, binprm->flags);
if (IS_ERR(file))
return PTR_ERR(file);
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..573ef06ff5a1 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -209,7 +209,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
if (!IS_ERR(interp_file))
deny_write_access(interp_file);
} else {
- interp_file = open_exec(fmt->interpreter);
+ interp_file = openat_exec(bprm->dfd, fmt->interpreter, bprm->flags);
}
retval = PTR_ERR(interp_file);
if (IS_ERR(interp_file))
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index d0078cbb718b..340f63635aac 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -102,7 +102,7 @@ static int load_script(struct linux_binprm *bprm)
/*
* OK, now restart the process with the interpreter's dentry.
*/
- file = open_exec(i_name);
+ file = openat_exec(bprm->dfd, i_name, bprm->flags);
if (IS_ERR(file))
return PTR_ERR(file);
diff --git a/fs/exec.c b/fs/exec.c
index bcf383730bea..e63063b2de23 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -846,12 +846,24 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
.lookup_flags = LOOKUP_FOLLOW,
};
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_BENEATH |
+ AT_XDEV | AT_NO_MAGICLINKS | AT_NO_SYMLINKS |
+ AT_THIS_ROOT)) != 0)
return ERR_PTR(-EINVAL);
if (flags & AT_SYMLINK_NOFOLLOW)
open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
if (flags & AT_EMPTY_PATH)
open_exec_flags.lookup_flags |= LOOKUP_EMPTY;
+ if (flags & AT_BENEATH)
+ open_exec_flags.lookup_flags |= LOOKUP_BENEATH;
+ if (flags & AT_XDEV)
+ open_exec_flags.lookup_flags |= LOOKUP_XDEV;
+ if (flags & AT_NO_MAGICLINKS)
+ open_exec_flags.lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (flags & AT_NO_SYMLINKS)
+ open_exec_flags.lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & AT_THIS_ROOT)
+ open_exec_flags.lookup_flags |= LOOKUP_IN_ROOT;
file = do_filp_open(fd, name, &open_exec_flags);
if (IS_ERR(file))
@@ -879,18 +891,18 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
return ERR_PTR(err);
}
-struct file *open_exec(const char *name)
+struct file *openat_exec(int dfd, const char *name, int flags)
{
struct filename *filename = getname_kernel(name);
struct file *f = ERR_CAST(filename);
if (!IS_ERR(filename)) {
- f = do_open_execat(AT_FDCWD, filename, 0);
+ f = do_open_execat(dfd, filename, flags);
putname(filename);
}
return f;
}
-EXPORT_SYMBOL(open_exec);
+EXPORT_SYMBOL(openat_exec);
int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)
@@ -1762,6 +1774,12 @@ static int __do_execve_file(int fd, struct filename *filename,
sched_exec();
+ bprm->flags = flags & (AT_XDEV | AT_NO_MAGICLINKS | AT_NO_SYMLINKS |
+ AT_THIS_ROOT);
+ bprm->dfd = AT_FDCWD;
+ if (bprm->flags)
+ bprm->dfd = fd;
+
bprm->file = file;
if (!filename) {
bprm->filename = "none";
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 688ab0de7810..e4da2d36e97f 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -50,6 +50,7 @@ struct linux_binprm {
unsigned int taso:1;
#endif
unsigned int recursion_depth; /* only for search_binary_handler() */
+ int dfd, flags; /* passed down to execat_open() */
struct file * file;
struct cred *cred; /* new credentials */
int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3e85cb8e8c20..a82c8dd44ad9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2937,8 +2937,13 @@ extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
-extern struct file * open_exec(const char *);
-
+
+extern struct file *openat_exec(int, const char *, int);
+static inline struct file *open_exec(const char *name)
+{
+ return openat_exec(AT_FDCWD, name, 0);
+}
+
/* fs/dcache.c -- generic fs support functions */
extern bool is_subdir(struct dentry *, struct dentry *);
extern bool path_is_under(const struct path *, const struct path *);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6448cdd9a350..607bc98813e3 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -90,5 +90,11 @@
#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+#define AT_RESOLUTION_TYPE 0xF8000 /* Type of path-resolution scoping we are applying. */
+#define AT_BENEATH 0x08000 /* - Block "lexical" trickery like "..", symlinks, absolute paths, etc. */
+#define AT_XDEV 0x10000 /* - Block mount-point crossings (includes bind-mounts). */
+#define AT_NO_MAGICLINKS 0x20000 /* - Block procfs-style "magic" symlinks. */
+#define AT_NO_SYMLINKS 0x40000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
+#define AT_THIS_ROOT 0x80000 /* - Scope ".." resolution to dirfd (like chroot(2)). */
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 4/5] namei: aggressively check for nd->root escape on ".." resolution
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Jann Horn, Kees Cook, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov,
Christian Brauner, David Drysdale, Chanho Min, Oleg Nesterov,
Aleksa Sarai, containers, linux-fsdevel, linux-api, linux-kernel,
linux-arch
In-Reply-To: <20190306191244.8691-1-cyphar@cyphar.com>
This patch allows for O_BENEATH and O_THISROOT to safely permit ".."
resolution (in the case of O_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
O_THISROOT will chroot(2)-style scope it). "magic link" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
O_THISROOT and O_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "magic link"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of O_THISROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of path_is_under() here might seem suspect, but on further
inspection of the most important race (a path was *inside* the root but
is now *outside*), there appears to be no attack potential. If
path_is_under() occurs before the rename, then the path will be resolved
but since the path was originally inside the root there is no escape.
Subsequent ".." jumps are guaranteed to check path_is_under() (by
construction, &rename_lock or &mount_lock must have been taken by the
attacker after path_is_under() returned in the victim), and thus will
not be able to escape from the previously-inside-root path. Walking down
is still safe since the entire subtree was moved (either by rename(2) or
MS_MOVE) and because (as discussed above) walking down is safe.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 48 +++++++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 798eb1702a0c..af06b10b13a0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -493,7 +493,7 @@ struct nameidata {
struct path root;
struct inode *inode; /* path.dentry.d_inode */
unsigned int flags;
- unsigned seq, m_seq;
+ unsigned seq, m_seq, r_seq;
int last_type;
unsigned depth;
int total_link_count;
@@ -1741,19 +1741,35 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
- /*
- * LOOKUP_BENEATH resolving ".." is not currently safe -- races can
- * cause our parent to have moved outside of the root and us to skip
- * over it.
- */
- if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
- return -EXDEV;
+ int error = 0;
+
if (!nd->root.mnt)
set_root(nd);
- if (nd->flags & LOOKUP_RCU) {
- return follow_dotdot_rcu(nd);
- } else
- return follow_dotdot(nd);
+ if (nd->flags & LOOKUP_RCU)
+ error = follow_dotdot_rcu(nd);
+ else
+ error = follow_dotdot(nd);
+ if (error)
+ return error;
+
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
+ bool m_retry = read_seqretry(&mount_lock, nd->m_seq);
+ bool r_retry = read_seqretry(&rename_lock, nd->r_seq);
+
+ /*
+ * Don't bother checking unless there's a racing
+ * rename(2) or MS_MOVE.
+ */
+ if (likely(!m_retry && !r_retry))
+ return 0;
+
+ if (m_retry && !(nd->flags & LOOKUP_RCU))
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (r_retry)
+ nd->r_seq = read_seqbegin(&rename_lock);
+ if (!path_is_under(&nd->path, &nd->root))
+ return -EXDEV;
+ }
}
return 0;
}
@@ -2274,6 +2290,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->last_type = LAST_ROOT; /* if there are only slashes... */
nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
nd->depth = 0;
+
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (unlikely(flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
+ nd->r_seq = read_seqbegin(&rename_lock);
+
if (flags & LOOKUP_ROOT) {
struct dentry *root = nd->root.dentry;
struct inode *inode = root->d_inode;
@@ -2284,7 +2305,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
if (flags & LOOKUP_RCU) {
nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
nd->root_seq = nd->seq;
- nd->m_seq = read_seqbegin(&mount_lock);
} else {
path_get(&nd->path);
}
@@ -2295,8 +2315,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.mnt = NULL;
nd->path.dentry = NULL;
- nd->m_seq = read_seqbegin(&mount_lock);
-
if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 3/5] namei: O_THISROOT: chroot-like path resolution
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, Kees Cook,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Jann Horn,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
containers, linux-fsdevel, linux-api, linux-kernel, linux-arch
In-Reply-To: <20190306191244.8691-1-cyphar@cyphar.com>
The primary motivation for the need for this flag is container runtimes
which have to interact with malicious root filesystems in the host
namespaces. One of the first requirements for a container runtime to be
secure against a malicious rootfs is that they correctly scope symlinks
(that is, they should be scoped as though they are chroot(2)ed into the
container's rootfs) and ".."-style paths[*]. The already-existing O_XDEV
and O_NOMAGICLINKS[**] help defend against other potential attacks in a
malicious rootfs scenario.
Currently most container runtimes try to do this resolution in
userspace[1], causing many potential race conditions. In addition, the
"obvious" alternative (actually performing a {ch,pivot_}root(2))
requires a fork+exec (for some runtimes) which is *very* costly if
necessary for every filesystem operation involving a container.
[*] At the moment, ".." and "magic link" jumping are disallowed for the
same reason it is disabled for O_BENEATH -- currently it is not safe
to allow it. Future patches may enable it unconditionally once we
have resolved the possible races (for "..") and semantics (for
"magic link" jumping).
The most significant openat(2) semantic change with O_THISROOT is that
absolute pathnames no longer cause dirfd to be ignored completely. The
rationale is that O_THISROOT must necessarily chroot-scope symlinks with
absolute paths to dirfd, and so doing it for the base path seems to be
the most consistent behaviour (and also avoids foot-gunning users who
want to scope paths that are absolute).
Currently this is only enabled for openat(2), and similar to O_BENEATH
and family requires more discussion about extending it to more *at(2)
syscalls as well as extending AT_EMPTY_PATH support.
[1]: https://github.com/cyphar/filepath-securejoin
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 6 +++---
fs/open.c | 4 +++-
include/linux/fcntl.h | 2 +-
include/linux/namei.h | 1 +
include/uapi/asm-generic/fcntl.h | 3 +++
6 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index f782e99700f0..a6b4565a903d 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(26 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index c9a07a8c005a..798eb1702a0c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1097,7 +1097,7 @@ const char *get_link(struct nameidata *nd)
if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
return ERR_PTR(-ELOOP);
/* Not currently safe. */
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return ERR_PTR(-EXDEV);
}
if (IS_ERR_OR_NULL(res))
@@ -1746,7 +1746,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
* cause our parent to have moved outside of the root and us to skip
* over it.
*/
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
@@ -2297,7 +2297,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
- if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
return ERR_PTR(error);
diff --git a/fs/open.c b/fs/open.c
index 3e73f940f56e..5914aed2fac8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -960,7 +960,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* cannot have anything other than the below set of flags
*/
flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
- O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS | O_THISROOT;
acc_mode = 0;
}
@@ -997,6 +997,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_NO_MAGICLINKS;
if (flags & O_NOSYMLINKS)
lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & O_THISROOT)
+ lookup_flags |= LOOKUP_IN_ROOT;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 864399c2fdd2..46c92bbfce4a 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
- O_NOMAGICLINKS | O_NOSYMLINKS)
+ O_NOMAGICLINKS | O_NOSYMLINKS | O_THISROOT)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 82b5039d27a6..0969313b518f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
Implies LOOKUP_NO_MAGICLINKS. */
+#define LOOKUP_IN_ROOT 0x100000 /* Treat dirfd as %current->fs->root. */
extern int path_pts(struct path *path);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index ba53e0836cd4..3d0fe0de2ba2 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -110,6 +110,9 @@
#ifndef O_NOSYMLINKS
#define O_NOSYMLINKS 01000000000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
#endif
+#ifndef O_THISROOT
+#define O_THISROOT 02000000000 /* - Scope ".." resolution to dirfd (like chroot(2)). */
+#endif
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 2/5] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, Kees Cook,
David Drysdale, Andy Lutomirski, Linus Torvalds, Andrew Morton,
Alexei Starovoitov, Jann Horn, Chanho Min, Oleg Nesterov,
Aleksa Sarai, containers, linux-fsdevel, linux-api, linux-kernel,
linux-arch
In-Reply-To: <20190306191244.8691-1-cyphar@cyphar.com>
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* O_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* O_NOMAGICLINKS: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* O_NOSYMLINKS: Disallows symlink jumping *of any kind*. Implies
O_NOMAGICLINKS (obviously).
* O_BENEATH: Disallow "escapes" from the starting point of the
filesystem tree during resolution (you must stay "beneath" the
starting point at all times). Currently this is done by disallowing
".." and absolute paths (either in the given path or found during
symlink resolution) entirely, as well as all "magic link" jumping.
The wholesale banning of ".." is because it is currently not safe to
allow ".." resolution (races can cause the path to be moved outside of
the root -- this is conceptually similar to historical chroot(2)
escape attacks). Future patches in this series will address this, and
will re-enable ".." resolution once it is safe. With those patches,
".." resolution will only be allowed if it remains in the root
throughout resolution (such as "a/../b" not "a/../../outside/b").
The banning of "magic link" jumping is done because it is not clear
whether semantically they should be allowed -- while some "magic
links" are safe there are many that can cause escapes (and once a
resolution is outside of the root, O_BENEATH will no longer detect
it). Future patches may re-enable "magic link" jumping when such jumps
would remain inside the root.
The O_NO*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2). In future it may be necessary to enable
these for *at(2) flags, as well as expanding support for AT_EMPTY_PATH.
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 76 +++++++++++++++++++++++++++-----
fs/open.c | 11 ++++-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 +++
include/uapi/asm-generic/fcntl.h | 14 ++++++
6 files changed, 99 insertions(+), 14 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..f782e99700f0 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index 4fdcb36f7c01..c9a07a8c005a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1053,6 +1059,9 @@ const char *get_link(struct nameidata *nd)
int error;
const char *res;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return ERR_PTR(-ELOOP);
+
if (!(nd->flags & LOOKUP_RCU)) {
touch_atime(&last->link);
cond_resched();
@@ -1083,14 +1092,23 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ /* If we just jumped it was because of a procfs-style link. */
+ if (unlikely(nd->flags & LOOKUP_JUMPED)) {
+ if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
+ return ERR_PTR(-ELOOP);
+ /* Not currently safe. */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return ERR_PTR(-EXDEV);
+ }
if (IS_ERR_OR_NULL(res))
return res;
}
if (*res == '/') {
if (!nd->root.mnt)
set_root(nd);
- if (unlikely(nd_jump_root(nd)))
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
while (unlikely(*++res == '/'))
;
}
@@ -1271,12 +1289,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1333,6 +1355,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1377,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *inode = nd->inode;
while (1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1379,6 +1406,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1393,6 +1422,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1481,8 +1512,11 @@ static int path_parent_directory(struct path *path)
static int follow_dotdot(struct nameidata *nd)
{
while(1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -1491,6 +1525,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1705,6 +1741,13 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ /*
+ * LOOKUP_BENEATH resolving ".." is not currently safe -- races can
+ * cause our parent to have moved outside of the root and us to skip
+ * over it.
+ */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
if (nd->flags & LOOKUP_RCU) {
@@ -2253,6 +2296,15 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ nd->root = nd->path;
+ if (!(nd->flags & LOOKUP_RCU))
+ path_get(&nd->root);
+ }
if (*s == '/') {
if (likely(!nd->root.mnt))
set_root(nd);
@@ -2261,9 +2313,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
s = ERR_PTR(error);
return s;
}
- error = dirfd_path_init(nd);
- if (unlikely(error))
- return ERR_PTR(error);
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
return s;
}
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..3e73f940f56e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -959,7 +959,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* If we have O_PATH in the open flag. Then we
* cannot have anything other than the below set of flags
*/
- flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
acc_mode = 0;
}
@@ -988,6 +989,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOMAGICLINKS)
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..864399c2fdd2 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOMAGICLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..82b5039d27a6 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_MAGICLINKS. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..ba53e0836cd4 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,20 @@
#define O_NDELAY O_NONBLOCK
#endif
+/* Type of path-resolution scoping we are applying. */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000 /* - Block "lexical" trickery like "..", symlinks, absolute paths, etc. */
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000 /* - Block mount-point crossings (includes bind-mounts). */
+#endif
+#ifndef O_NOMAGICLINKS
+#define O_NOMAGICLINKS 00200000000 /* - Block procfs-style "magic" symlinks. */
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 1/5] namei: split out nd->dfd handling to dirfd_path_init
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Christian Brauner,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
containers, linux-fsdevel, linux-api, linux-kernel, linux-arch
In-Reply-To: <20190306191244.8691-1-cyphar@cyphar.com>
Previously, path_init's handling of *at(dfd, ...) was only done once,
but with O_BENEATH (and O_THISROOT) we have to parse the initial
nd->path at different times (before or after absolute path handling)
depending on whether we have been asked to scope resolution within a
root.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 103 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 59 insertions(+), 44 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a85deb55d0c9..4fdcb36f7c01 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2168,9 +2168,59 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ do {
+ seq = read_seqcount_begin(&fs->seq);
+ nd->path = fs->pwd;
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
+ } while (read_seqcount_retry(&fs->seq, seq));
+ } else {
+ get_fs_pwd(current->fs, &nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ } else {
+ /* Caller must check execute permissions on the starting path component */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *dentry;
+
+ if (!f.file)
+ return -EBADF;
+
+ dentry = f.file->f_path.dentry;
+
+ if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
+ fdput(f);
+ return -ENOTDIR;
+ }
+
+ nd->path = f.file->f_path;
+ if (nd->flags & LOOKUP_RCU) {
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+ } else {
+ path_get(&nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ fdput(f);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
@@ -2204,52 +2254,17 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- do {
- seq = read_seqcount_begin(&fs->seq);
- nd->path = fs->pwd;
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
- } while (read_seqcount_retry(&fs->seq, seq));
- } else {
- get_fs_pwd(current->fs, &nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_PTR(-ENOTDIR);
- }
-
- nd->path = f.file->f_path;
- if (flags & LOOKUP_RCU) {
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
- } else {
- path_get(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- fdput(f);
+ if (likely(!nd->root.mnt))
+ set_root(nd);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 0/5] namei: vfs flags to restrict path resolution
From: Aleksa Sarai @ 2019-03-06 19:12 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Jann Horn,
Christian Brauner, David Drysdale, Tycho Andersen, Kees Cook,
containers, linux-fsdevel, linux-api, Andrew Morton,
Alexei Starovoitov, Chanho Min, Oleg Nesterov, Aleksa Sarai,
linux-kernel, linux-arch
Now that the holiday break is over, it's time to re-send this patch
series (with a few additions, due to new information we got from
CVE-2019-5736 -- which this patchset mostly protected against but had
some holes with regards to #!-style scripts).
Patch changelog:
v5:
* In response to CVE-2019-5736 (one of the vectors showed that
open(2)+fexec(3) cannot be used to scope binfmt_script's implicit
open_exec()), AT_* flags have been re-added and are now piped
through to binfmt_script (and other binfmt_* that use open_exec)
but are only supported for execveat(2) for now.
v4:
* Remove AT_* flag reservations, as they require more discussion.
* Switch to path_is_under() over __d_path() for breakout checking.
* Make O_XDEV no longer block openat("/tmp", "/", O_XDEV) -- dirfd
is now ignored for absolute paths to match other flags.
* Improve the dirfd_path_init() refactor and move it to a separate
commit.
* Remove reference to Linux-capsicum.
* Switch "proclink" name to "magic link".
v3: [resend]
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
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.
In line with the conclusions of the original discussion of AT_NO_JUMPS,
the flag has been split up into separate flags:
* O_XDEV blocks all mountpoint crossings (upwards, downwards, or
through absolute links). Absolute pathnames alone in openat(2) do
not trigger this.
* O_NOMAGICLINKS blocks resolution through /proc/$pid/fd-style links.
This is done by blocking the usage of nd_jump_link() during
resolution in a filesystem. The term "magic links" is used to match
with the only reference to these links in Documentation/, but I'm
happy to change the name.
It should be noted that this is different to the scope of O_NOFOLLOW
in that it applies to all path components. However, you can do
open(O_NOFOLLOW|O_NOMAGICLINKS|O_PATH) on a "magic link" and it will
*not* fail (assuming that no parent component was a "magic link"),
and you will have an fd for the "magic link".
* O_BENEATH disallows escapes to outside the starting dirfd's tree,
using techniques such as ".." or absolute links. Absolute paths in
openat(2) are also disallowed. Conceptually this flag is to ensure
you "stay below" a certain point in the filesystem tree -- but this
requires some additional to protect against various races that would
allow escape using ".." (see patch 4 for more detail).
Currently O_BENEATH implies O_NOMAGICLINKS, because it can trivially
beam you around the filesystem (breaking the protection). In future,
there might be similar safety checks as in patch 4, but that
requires more discussion.
In addition, two new flags were added that expand on the above ideas:
* O_NOSYMLINKS does what it says on the tin. No symlink resolution is
allowed at all, including "magic links". Just as with O_NOMAGICLINKS
this can still be used with (O_PATH|O_NOFOLLOW) to open an fd for
the symlink as long as no parent path had a symlink component.
* O_THISROOT is an extension of O_BENEATH that, rather than blocking
attempts to move past the root, forces all such movements to be
scoped to the starting point. This provides chroot(2)-like
protection but without the cost of a chroot(2) for each filesystem
operation, as well as being safe against race attacks that chroot(2)
is not.
If a race is detected (as with O_BENEATH) then an error is
generated, and similar to O_BENEATH it is not permitted to cross
"magic links" with O_THISROOT.
The primary need for this is from container runtimes, which
currently need to do symlink scoping in userspace[6] when opening
paths in a potentially malicious container. There is a long list of
CVEs that could have bene mitigated by having O_THISROOT (such as
CVE-2017-1002101, CVE-2017-1002102, CVE-2018-15664, and
CVE-2019-5736, just to name a few).
In addition, a mirror set of AT_* flags have been added (though
currently these are only supported for execveat(2) -- and not for any
other syscall). The need for these is explained in the final patch in
the series (it's motivated by CVE-2019-5736).
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <drysdale@google.com>
Cc: Tycho Andersen <tycho@tycho.ws>
Cc: Kees Cook <keescook@chromium.org>
Cc: <containers@lists.linux-foundation.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-api@vger.kernel.org>
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Aleksa Sarai (5):
namei: split out nd->dfd handling to dirfd_path_init
namei: O_BENEATH-style path resolution flags
namei: O_THISROOT: chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
binfmt_*: scope path resolution of interpreters
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/binfmt_em86.c | 4 +-
fs/binfmt_misc.c | 2 +-
fs/binfmt_script.c | 2 +-
fs/exec.c | 26 +++-
fs/fcntl.c | 2 +-
fs/namei.c | 205 ++++++++++++++++++++++---------
fs/open.c | 13 +-
include/linux/binfmts.h | 1 +
include/linux/fcntl.h | 3 +-
include/linux/fs.h | 9 +-
include/linux/namei.h | 8 ++
include/uapi/asm-generic/fcntl.h | 17 +++
include/uapi/linux/fcntl.h | 6 +
15 files changed, 228 insertions(+), 74 deletions(-)
--
2.21.0
^ permalink raw reply
* Re: a.out coredumping: fix or delete?
From: Andy Lutomirski @ 2019-03-06 17:45 UTC (permalink / raw)
To: Alan Cox
Cc: Theodore Y. Ts'o, Thomas Gleixner, Matthew Wilcox,
Borislav Petkov, Jann Horn, Al Viro, kernel list, Linux FS Devel,
the arch/x86 maintainers, Linux API
In-Reply-To: <20190306165230.75b7d110@alans-desktop>
On Wed, Mar 6, 2019 at 8:53 AM Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>
> On Wed, 6 Mar 2019 09:11:44 -0500
> "Theodore Y. Ts'o" <tytso@mit.edu> wrote:
>
> > On Wed, Mar 06, 2019 at 01:25:17PM +0100, Thomas Gleixner wrote:
> > > > It's been 25 years since Linux added support for ELF. Can we just
> > > > delete the a.out support entirely now? According to the Linux-ELF HOWTO,
> > > > support was added in 1.1.52 (August 1994). It's pretty much necromancy
> > > > at this point.
> > >
> > > The Kernel-Necrophilia cult members might disagree. :)
> > >
> > > But yes, good riddance.
> >
> > Doesn't Minix 1.0 use a.out? It *is* cool to be able to binaries from
> > run dead operating systems. :-)
>
> Minixemu compiled fine ELF last time I checked 8). It does need a 32bit
> system as it still uses virtual 86 model.
>
While I consider vm86() to be a giant turd, I also consider it to be
supported and even wrote it a somewhat decent test suite :)
^ permalink raw reply
* Re: [PATCH v7 10/15] sched/fair: uclamp: Add uclamp support to energy_compute()
From: Quentin Perret @ 2019-03-06 17:21 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190208100554.32196-11-patrick.bellasi@arm.com>
Hi Patrick,
On Friday 08 Feb 2019 at 10:05:49 (+0000), Patrick Bellasi wrote:
> The Energy Aware Scheduler (AES) estimates the energy impact of waking
s/AES/EAS
> up a task on a given CPU. This estimation is based on:
> a) an (active) power consumptions defined for each CPU frequency
s/consumptions/consumption
> b) an estimation of which frequency will be used on each CPU
> c) an estimation of the busy time (utilization) of each CPU
>
> Utilization clamping can affect both b) and c) estimations. A CPU is
> expected to run:
> - on an higher than required frequency, but for a shorter time, in case
> its estimated utilization will be smaller then the minimum utilization
s/then/than
> enforced by uclamp
> - on a smaller than required frequency, but for a longer time, in case
> its estimated utilization is bigger then the maximum utilization
s/then/than
> enforced by uclamp
>
> While effects on busy time for both boosted/capped tasks are already
> considered by compute_energy(), clamping effects on frequency selection
> are currently ignored by that function.
>
> Fix it by considering how CPU clamp values will be affected by a
> task waking up and being RUNNABLE on that CPU.
>
> Do that by refactoring schedutil_freq_util() to take an additional
> task_struct* which allows EAS to evaluate the impact on clamp values of
> a task being eventually queued in a CPU. Clamp values are applied to the
> RT+CFS utilization only when a FREQUENCY_UTIL is required by
> compute_energy().
>
> Do note that switching from ENERGY_UTIL to FREQUENCY_UTIL in the
> computation of cpu_util signal implies that we are more likely to
> estimate the higherst OPP when a RT task is running in another CPU of
s/higherst/highest
> the same performance domain. This can have an impact on energy
> estimation but:
> - it's not easy to say which approach is better, since it quite likely
> depends on the use case
> - the original approach could still be obtained by setting a smaller
> task-specific util_min whenever required
>
> Since we are at that:
> - rename schedutil_freq_util() into schedutil_cpu_util(),
> since it's not only used for frequency selection.
> - use "unsigned int" instead of "unsigned long" whenever the tracked
> utilization value is not expected to overflow 32bit.
We use unsigned long all over the place right ? All the task_util*()
functions return unsigned long, the capacity-related functions too, and
util_avg is an unsigned long in sched_avg. So I'm not sure if we want to
do this TBH.
> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> ---
> Changes in v7:
> Message-ID: <20190122151404.5rtosic6puixado3@queper01-lin>
> - add a note on side-effects due to the usage of FREQUENCY_UTIL for
> performance domain frequency estimation
> - add a similer note to this changelog
> ---
> kernel/sched/cpufreq_schedutil.c | 18 ++++++++-------
> kernel/sched/fair.c | 39 +++++++++++++++++++++++++++-----
> kernel/sched/sched.h | 18 ++++-----------
> 3 files changed, 48 insertions(+), 27 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 70a8b87fa29c..fdad719fca8b 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -195,10 +195,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> * based on the task model parameters and gives the minimal utilization
> * required to meet deadlines.
> */
> -unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> - unsigned long max, enum schedutil_type type)
> +unsigned int schedutil_cpu_util(int cpu, unsigned int util_cfs,
> + unsigned int max, enum schedutil_type type,
> + struct task_struct *p)
> {
> - unsigned long dl_util, util, irq;
> + unsigned int dl_util, util, irq;
> struct rq *rq = cpu_rq(cpu);
>
> if (!IS_BUILTIN(CONFIG_UCLAMP_TASK) &&
> @@ -229,7 +230,7 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> */
> util = util_cfs + cpu_util_rt(rq);
> if (type == FREQUENCY_UTIL)
> - util = uclamp_util(rq, util);
> + util = uclamp_util_with(rq, util, p);
>
> dl_util = cpu_util_dl(rq);
>
> @@ -283,13 +284,14 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> {
> struct rq *rq = cpu_rq(sg_cpu->cpu);
> - unsigned long util = cpu_util_cfs(rq);
> - unsigned long max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
> + unsigned int util_cfs = cpu_util_cfs(rq);
> + unsigned int cpu_cap = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
Do you really need this one ? What's wrong with 'max' :-) ?
> - sg_cpu->max = max;
> + sg_cpu->max = cpu_cap;
> sg_cpu->bw_dl = cpu_bw_dl(rq);
>
> - return schedutil_freq_util(sg_cpu->cpu, util, max, FREQUENCY_UTIL);
> + return schedutil_cpu_util(sg_cpu->cpu, util_cfs, cpu_cap,
> + FREQUENCY_UTIL, NULL);
> }
>
> /**
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8c0aa76af90a..f6b0808e01ad 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6453,11 +6453,20 @@ static unsigned long cpu_util_next(int cpu, struct task_struct *p, int dst_cpu)
> static long
> compute_energy(struct task_struct *p, int dst_cpu, struct perf_domain *pd)
> {
> - long util, max_util, sum_util, energy = 0;
> + unsigned int max_util, cfs_util, cpu_util, cpu_cap;
> + unsigned long sum_util, energy = 0;
> int cpu;
>
> for (; pd; pd = pd->next) {
> + struct cpumask *pd_mask = perf_domain_span(pd);
> +
> + /*
> + * The energy model mandate all the CPUs of a performance
s/mandate/mandates
> + * domain have the same capacity.
> + */
> + cpu_cap = arch_scale_cpu_capacity(NULL, cpumask_first(pd_mask));
> max_util = sum_util = 0;
> +
> /*
> * The capacity state of CPUs of the current rd can be driven by
> * CPUs of another rd if they belong to the same performance
> @@ -6468,11 +6477,29 @@ compute_energy(struct task_struct *p, int dst_cpu, struct perf_domain *pd)
> * it will not appear in its pd list and will not be accounted
> * by compute_energy().
> */
> - for_each_cpu_and(cpu, perf_domain_span(pd), cpu_online_mask) {
> - util = cpu_util_next(cpu, p, dst_cpu);
> - util = schedutil_energy_util(cpu, util);
> - max_util = max(util, max_util);
> - sum_util += util;
> + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> +
> + /*
> + * Busy time computation: utilization clamping is not
> + * required since the ratio (sum_util / cpu_capacity)
> + * is already enough to scale the EM reported power
> + * consumption at the (eventually clamped) cpu_capacity.
> + */
> + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> + ENERGY_UTIL, NULL);
> +
> + /*
> + * Performance domain frequency: utilization clamping
> + * must be considered since it affects the selection
> + * of the performance domain frequency.
> + * NOTE: in case RT tasks are running, by default the
> + * FREQUENCY_UTIL's utilization can be max OPP.
> + */
> + cpu_util = schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> + FREQUENCY_UTIL,
> + cpu == dst_cpu ? p : NULL);
> + max_util = max(max_util, cpu_util);
> }
>
> energy += em_pd_energy(pd->em_pd, max_util, sum_util);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index de181b8a3a2a..b9acef080d99 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2335,6 +2335,7 @@ static inline unsigned long capacity_orig_of(int cpu)
> #endif
>
> #ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL
> +
> /**
> * enum schedutil_type - CPU utilization type
Since you're using this enum unconditionally in fair.c, you should to
move it out of the #ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL block, I think.
> * @FREQUENCY_UTIL: Utilization used to select frequency
> @@ -2350,15 +2351,9 @@ enum schedutil_type {
> ENERGY_UTIL,
> };
>
> -unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> - unsigned long max, enum schedutil_type type);
> -
> -static inline unsigned long schedutil_energy_util(int cpu, unsigned long cfs)
> -{
> - unsigned long max = arch_scale_cpu_capacity(NULL, cpu);
> -
> - return schedutil_freq_util(cpu, cfs, max, ENERGY_UTIL);
> -}
> +unsigned int schedutil_cpu_util(int cpu, unsigned int util_cfs,
> + unsigned int max, enum schedutil_type type,
> + struct task_struct *p);
>
> static inline unsigned long cpu_bw_dl(struct rq *rq)
> {
> @@ -2387,10 +2382,7 @@ static inline unsigned long cpu_util_rt(struct rq *rq)
> return READ_ONCE(rq->avg_rt.util_avg);
> }
> #else /* CONFIG_CPU_FREQ_GOV_SCHEDUTIL */
> -static inline unsigned long schedutil_energy_util(int cpu, unsigned long cfs)
> -{
> - return cfs;
> -}
> +#define schedutil_cpu_util(cpu, util_cfs, max, type, p) 0
> #endif
>
> #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
> --
> 2.20.1
>
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH for 5.1 0/3] Restartable Sequences updates for 5.1
From: Mathieu Desnoyers @ 2019-03-06 17:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: H.J. Lu, libc-alpha, Thomas Gleixner, linux-kernel, linux-api,
Paul E . McKenney, Boqun Feng, 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: <20190306083039.GS32477@hirez.programming.kicks-ass.net>
----- On Mar 6, 2019, at 3:30 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Mar 05, 2019 at 05:32:10PM -0500, Mathieu Desnoyers wrote:
>
>> >> * Adaptative mutex improvements
>> >>
>> >> I have done a prototype using rseq to implement an adaptative mutex which
>> >> can detect preemption using a rseq critical section. This ensures the
>> >> thread doesn't continue to busy-loop after it returns from preemption, and
>> >> calls sys_futex() instead. This is part of a user-space prototype branch [2],
>> >> and does not require any kernel change.
>> >
>> > I'm still not convinced that is actually the right way to go about
>> > things. The kernel heuristic is spin while the _owner_ runs, and we
>> > don't get preempted, obviously.
>> >
>> > And the only userspace spinning that makes sense is to cover the cost of
>> > the syscall. Now Obviously PTI wrecked everything, but before that
>> > syscalls were actually plenty fast and you didn't need many cmpxchg
>> > cycles to amortize the syscall itself -- which could then do kernel side
>> > adaptive spinning (when required).
>>
>> Indeed with PTI the system calls are back to their slow self. ;)
>>
>> You point about owner is interesting. Perhaps there is one tweak that I
>> should add in there. We could write the owner thread ID in the lock word.
>
> This is already required for PI (and I think robust) futexes. There have
> been proposals for FUTEX_LOCK and FUTEX_UNLOCK (!PI) primitives that
> require the same.
>
> Waiman had some patches; but I think all went under because 'important'
> stuff happened.
>
>> When trying to grab a lock, one of a few situations can happen:
>>
>> - It's unlocked, so we grab it by storing our thread ID,
>> - It's locked, and we can fetch the CPU number of the thread owning it
>> if we can access its (struct rseq *)->cpu_id through a lookup using its
>> thread ID, We can then check whether it's the same CPU we are running on.
>
> That might just work with threads (private futexes; which are the
> majority these these I think), but will obviously not work with regular
> (shared) futexes.
If we have enough space available either in the lock word or just nearby,
we could write the CPU number that was current when the thread owning
the lock grabbed it. Considering that it should be infrequent that threads
get migrated to other CPUs while holding the lock, it might be a good enough
heuristic to figure out whether a thread needs to busy-wait or immediately
call futex.
Writing the CPU number would work both for private and shared futexes.
>
>> - If so, we _know_ we should let the owner run, so we call futex right away,
>> no spinning. We can even boost it for priority inheritance mutexes,
>> - If it's owned by a thread which was last running on a different CPU,
>> then it may make sense to actively try to grab the lock by spinning
>> up to a certain number of loops (which can be either fixed or adaptative).
>> After that limit, call futex. If preempted while looping, call futex.
>>
>> Do you see this as an improvement over what exists today, or am I
>> on the wrong track ?
>
> That's probably better than what they have today. Last time I looked at
> libc pthread I got really sad -- arguably that was a long time ago, and
> some of that stuff is because POSIX, but still.
>
> Some day we should redesign all that.. futex2 etc.
It sounds like an interesting topic to bring up at the next LPC! In the
meantime, a good start would be to state the desiderata of what requirements
should be covered by this redesign.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] x86: Deprecate a.out support
From: Enrico Weigelt, metux IT consult @ 2019-03-06 16:55 UTC (permalink / raw)
To: Linus Torvalds, Borislav Petkov
Cc: Alan Cox, Matthew Wilcox, Jann Horn, Al Viro, Thomas Gleixner,
kernel list, linux-fsdevel, the arch/x86 maintainers, Linux API,
Andrew Morton, Richard Weinberger, Anton Ivanov
In-Reply-To: <CAHk-=wi2E4kugAW7BQwNSVwYHOr=B_9EgzBRMpQ2+1dxAqydDg@mail.gmail.com>
On 05.03.19 17:22, Linus Torvalds wrote:
> On Tue, Mar 5, 2019 at 6:59 AM Borislav Petkov <bp@alien8.de> wrote:
>>
>> We can at least deprecate it on x86...
>
> I'd prefer to try to deprecate a.out core dumping first.. That's the
> part that is actually broken, no?
>
> In fact, I'd be happy to deprecate a.out entirely, but if somebody
> _does_ complain, I'd like to be able to bring it back without the core
> dumping.
Maybe make it depend on BROKEN ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* [PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Alexey Gladkov @ 2019-03-06 16:54 UTC (permalink / raw)
To: linux-kernel, linux-kbuild, linux-api
Cc: Kirill A . Shutemov, Gleb Fotengauer-Malinovskiy, Dmitry V. Levin
[-- Attachment #1: Type: text/plain, Size: 4702 bytes --]
Problem:
When a kernel module is compiled as a separate module, some important
information about the kernel module is available via .modinfo section of
the module. In contrast, when the kernel module is compiled into the
kernel, that information is not available.
Information about built-in modules is necessary in the following cases:
1. When it is necessary to find out what additional parameters can be
passed to the kernel at boot time.
2. When you need to know which module names and their aliases are in
the kernel. This is very useful for creating an initrd image.
Proposal:
The proposed patch does not remove .modinfo section with module
information from the vmlinux at the build time and saves it into a
separate file after kernel linking. So, the kernel does not increase in
size and no additional information remains in it. Information is stored
in the same format as in the separate modules (null-terminated string
array). Because the .modinfo section is already exported with a separate
modules, we are not creating a new API.
It can be easily read in the userspace:
$ tr '\0' '\n' < kernel.builtin.modinfo
ext4.softdep=pre: crc32c
ext4.license=GPL
ext4.description=Fourth Extended Filesystem
ext4.author=Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others
ext4.alias=fs-ext4
ext4.alias=ext3
ext4.alias=fs-ext3
ext4.alias=ext2
ext4.alias=fs-ext2
md_mod.alias=block-major-9-*
md_mod.alias=md
md_mod.description=MD RAID framework
md_mod.license=GPL
md_mod.parmtype=create_on_open:bool
md_mod.parmtype=start_dirty_degraded:int
...
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
---
Makefile | 1 +
include/linux/moduleparam.h | 12 +++++-------
scripts/link-vmlinux.sh | 8 ++++++++
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/Makefile b/Makefile
index d5713e7b1e50..971102194c92 100644
--- a/Makefile
+++ b/Makefile
@@ -1288,6 +1288,7 @@ _modinst_:
fi
@cp -f $(objtree)/modules.order $(MODLIB)/
@cp -f $(objtree)/modules.builtin $(MODLIB)/
+ @cp -f $(objtree)/kernel.builtin.modinfo $(MODLIB)/
$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
# This depmod is only for convenience to give the initial
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index ba36506db4fb..5ba250d9172a 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -10,23 +10,21 @@
module name. */
#ifdef MODULE
#define MODULE_PARAM_PREFIX /* empty */
+#define __MODULE_INFO_PREFIX /* empty */
#else
#define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
+/* We cannot use MODULE_PARAM_PREFIX because some modules override it. */
+#define __MODULE_INFO_PREFIX KBUILD_MODNAME "."
#endif
/* Chosen so that structs with an unsigned long line up. */
#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
-#ifdef MODULE
#define __MODULE_INFO(tag, name, info) \
static const char __UNIQUE_ID(name)[] \
__used __attribute__((section(".modinfo"), unused, aligned(1))) \
- = __stringify(tag) "=" info
-#else /* !MODULE */
-/* This struct is here for syntactic coherency, it is not used */
-#define __MODULE_INFO(tag, name, info) \
- struct __UNIQUE_ID(name) {}
-#endif
+ = __MODULE_INFO_PREFIX __stringify(tag) "=" info
+
#define __MODULE_PARM_TYPE(name, _type) \
__MODULE_INFO(parmtype, name##type, #name ":" _type)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index c8cf45362bd6..399d7e4d11ec 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -258,10 +258,12 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
# step 1
vmlinux_link "" .tmp_vmlinux1
+ "${OBJCOPY}" -R .modinfo .tmp_vmlinux1
kallsyms .tmp_vmlinux1 .tmp_kallsyms1.o
# step 2
vmlinux_link .tmp_kallsyms1.o .tmp_vmlinux2
+ "${OBJCOPY}" -R .modinfo .tmp_vmlinux2
kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o
# step 3
@@ -273,6 +275,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
kallsyms_vmlinux=.tmp_vmlinux3
vmlinux_link .tmp_kallsyms2.o .tmp_vmlinux3
+ "${OBJCOPY}" -R .modinfo .tmp_vmlinux3
kallsyms .tmp_vmlinux3 .tmp_kallsyms3.o
fi
@@ -281,6 +284,11 @@ fi
info LD vmlinux
vmlinux_link "${kallsymso}" vmlinux
+info MODINFO kernel.builtin.modinfo
+"${OBJCOPY}" -j .modinfo -O binary vmlinux kernel.builtin.modinfo
+chmod 444 kernel.builtin.modinfo
+"${OBJCOPY}" -R .modinfo vmlinux
+
if [ -n "${CONFIG_BUILDTIME_EXTABLE_SORT}" ]; then
info SORTEX vmlinux
sortextable vmlinux
--
2.19.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply related
* Re: a.out coredumping: fix or delete?
From: Alan Cox @ 2019-03-06 16:52 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Thomas Gleixner, Matthew Wilcox, Borislav Petkov, Jann Horn,
Al Viro, kernel list, linux-fsdevel, the arch/x86 maintainers,
Linux API
In-Reply-To: <20190306141144.GD7930@mit.edu>
On Wed, 6 Mar 2019 09:11:44 -0500
"Theodore Y. Ts'o" <tytso@mit.edu> wrote:
> On Wed, Mar 06, 2019 at 01:25:17PM +0100, Thomas Gleixner wrote:
> > > It's been 25 years since Linux added support for ELF. Can we just
> > > delete the a.out support entirely now? According to the Linux-ELF HOWTO,
> > > support was added in 1.1.52 (August 1994). It's pretty much necromancy
> > > at this point.
> >
> > The Kernel-Necrophilia cult members might disagree. :)
> >
> > But yes, good riddance.
>
> Doesn't Minix 1.0 use a.out? It *is* cool to be able to binaries from
> run dead operating systems. :-)
Minixemu compiled fine ELF last time I checked 8). It does need a 32bit
system as it still uses virtual 86 model.
Alan
^ permalink raw reply
* Re: [PATCH] x86: Deprecate a.out support
From: Geert Uytterhoeven @ 2019-03-06 15:07 UTC (permalink / raw)
To: Borislav Petkov
Cc: Linus Torvalds, Alan Cox, Matthew Wilcox, Jann Horn, Al Viro,
Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API, Andrew Morton,
Richard Weinberger, Anton Ivanov, alpha, linux-m68k
In-Reply-To: <20190305181808.GH8256@zn.tnic>
Hi Borislav,
On Tue, Mar 5, 2019 at 8:04 PM Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Mar 05, 2019 at 07:11:38PM +0100, Borislav Petkov wrote:
> > I guess you could Cc arch maintainers with the a.out-core.h removal
> > patch to see if anyone screams.
>
> And they're like two for which we need confirmation:
>
> $ git ls-files | grep a.out-core.h
> arch/alpha/include/asm/a.out-core.h
> arch/m68k/include/asm/a.out-core.h
> arch/um/include/asm/a.out-core.h
> arch/x86/include/asm/a.out-core.h
>
> um and x86 are clear.
>
> Adding alpha and m68k MLs to Cc.
Thanks!
The oldest binaries I still have lying around (an ext2 ramdisk image,
still used from time to time) are ELF, from just after the a.out to ELF
transition on m68k.
I think it's safe to assume no one still runs a.out binaries on m68k.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ 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