* Re: [PATCH v3 4/7] dcache: allocate external names from reclaimable kmalloc caches
From: Mel Gorman @ 2018-07-19 8:42 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, Christoph Lameter, David Rientjes,
Joonsoo Kim, Matthew Wilcox
In-Reply-To: <20180718133620.6205-5-vbabka@suse.cz>
On Wed, Jul 18, 2018 at 03:36:17PM +0200, Vlastimil Babka wrote:
> We can use the newly introduced kmalloc-reclaimable-X caches, to allocate
> external names in dcache, which will take care of the proper accounting
> automatically, and also improve anti-fragmentation page grouping.
>
> This effectively reverts commit f1782c9bc547 ("dcache: account external names
> as indirectly reclaimable memory") and instead passes __GFP_RECLAIMABLE to
> kmalloc(). The accounting thus moves from NR_INDIRECTLY_RECLAIMABLE_BYTES to
> NR_SLAB_RECLAIMABLE, which is also considered in MemAvailable calculation and
> overcommit decisions.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 7/7] mm, slab: shorten kmalloc cache names for large sizes
From: Mel Gorman @ 2018-07-19 8:46 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, Christoph Lameter, David Rientjes,
Joonsoo Kim, Matthew Wilcox
In-Reply-To: <20180718133620.6205-8-vbabka@suse.cz>
On Wed, Jul 18, 2018 at 03:36:20PM +0200, Vlastimil Babka wrote:
> Kmalloc cache names can get quite long for large object sizes, when the sizes
> are expressed in bytes. Use 'k' and 'M' prefixes to make the names as short
> as possible e.g. in /proc/slabinfo. This works, as we mostly use power-of-two
> sizes, with exceptions only below 1k.
>
> Example: 'kmalloc-4194304' becomes 'kmalloc-4M'
>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
There is a slight chance this will break any external tooling that
calculates fragmentation stats for slab/slub if they are particularly
stupid parsers but other than that;
Acked-by: Mel Gorman <mgorman@techsingularity.net>
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 02/17] y2038: Remove newstat family from default syscall set
From: Arnd Bergmann @ 2018-07-19 8:53 UTC (permalink / raw)
To: Joseph Myers
Cc: Christoph Hellwig, Thomas Gleixner, y2038 Mailman List, Linux API,
linux-arch, GNU C Library, Albert ARIBAUD, Networking, Al Viro,
Peter Zijlstra, Darren Hart, Eric W . Biederman,
Dominik Brodowski
In-Reply-To: <alpine.DEB.2.20.1807182010290.7221@digraph.polyomino.org.uk>
On Wed, Jul 18, 2018 at 10:15 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 17 Jul 2018, Arnd Bergmann wrote:
>
>> That would definitely help. See below for the stat implementation
>> I did in my musl libc prototype based on statx(). It passes the
>> LTP syscall tests, but that doesn't mean all the corner cases
>> are correct.
>
> Well, you definitely need explicit timestamp conversions on the result of
> statx to be usable in struct stat when userspace "long" is 64-bit, for BE
> because otherwise the integer nanoseconds will be in the wrong place for
> struct timespec, and for LE if the "__reserved is held in case we need a
> yet finer resolution." might start being returned as nonzero (for integer
> attoseconds, I suppose) in future.
Right. The musl implementation I sent did that by simply copying each
field we care about from the statx structure into the stat structure
individually. This is the more or less the same thing that the kernel
does to implement stat() as well.
Arnd
^ permalink raw reply
* Re: [PATCH 0/6 v1 resend] statfs: handle mount propagation
From: Christian Brauner @ 2018-07-19 15:30 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel, linux-fsdevel, torvalds, tglx, kstewart,
pombredanne, linux-api, ebiederm, seth.forshee, viro
In-Reply-To: <20180613150702.GA5403@mailbox.org>
On Wed, Jun 13, 2018 at 05:07:02PM +0200, Christian Brauner wrote:
> On Wed, Jun 13, 2018 at 04:24:54PM +0200, Greg KH wrote:
> > On Wed, Jun 13, 2018 at 04:06:25PM +0200, Christian Brauner wrote:
> > > On Fri, May 25, 2018 at 02:48:19PM +0200, Christian Brauner wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > >
> > > > Hey,
> > > >
> > > > This is v1 of this patchset. All changes from v0 to v1 are non-functional.
> > > > Specifically, the commit messages and justification have been extended as
> > > > requested by Linus and Al.
> > >
> > > Hey everyone,
> > >
> > > Just a ping whether the requested changes have made this series suitable
> > > for inclusion. Would be excellent if someone could do another review. :)
> >
> > It's the middle of the merge window, no new code gets accepted right
> > now, so just wait until next week at the _earliest_ before worrying
>
> Ok, sorry. It was just that this was sent out before well over a month
> ago and I was just worried that it had been overlooked.
Hey everyone,
It's been a few weeks. I just wanted to check whether the changes made
to the series would make it suitable for inclusion. I checked against
current master and it should still apply cleanly.
Thanks!
Christian
>
> Thanks!
> Christian
>
> > about someone reviewing this.
> >
> > thanks,
> >
> > greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH v2 16/27] mm: Modify can_follow_write_pte/pmd for shadow stack
From: Yu-cheng Yu @ 2018-07-19 17:06 UTC (permalink / raw)
To: Dave Hansen, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <f4c90626-51d8-5551-5b77-baaff81f16bb@linux.intel.com>
On Wed, 2018-07-18 at 17:06 -0700, Dave Hansen wrote:
> >
> > >
> > > >
> > > > -static inline bool can_follow_write_pte(pte_t pte, unsigned
> > > > int flags)
> > > > +static inline bool can_follow_write(pte_t pte, unsigned int
> > > > flags,
> > > > + struct vm_area_struct
> > > > *vma)
> > > > {
> > > > - return pte_write(pte) ||
> > > > - ((flags & FOLL_FORCE) && (flags & FOLL_COW)
> > > > && pte_dirty(pte));
> > > > + if (!is_shstk_mapping(vma->vm_flags)) {
> > > > + if (pte_write(pte))
> > > > + return true;
> > > Let me see if I can say this another way.
> > >
> > > The bigger issue is that these patches change the semantics of
> > > pte_write(). Before these patches, it meant that you *MUST*
> > > have this
> > > bit set to write to the page controlled by the PTE. Now, it
> > > means: you
> > > can write if this bit is set *OR* the shadowstack bit
> > > combination is set.
> > Here, we only figure out (1) if the page is pointed by a writable
> > PTE; or
> > (2) if the page is pointed by a RO PTE (data or SHSTK) and it has
> > been
> > copied and it still exists. We are not trying to
> > determine if the
> > SHSTK PTE is writable (we know it is not).
> Please think about the big picture. I'm not just talking about this
> patch, but about every use of pte_write() in the kernel.
>
> >
> > >
> > > That's the fundamental problem. We need some code in the kernel
> > > that
> > > logically represents the concept of "is this PTE a shadowstack
> > > PTE or a
> > > PTE with the write bit set", and we will call that pte_write(),
> > > or maybe
> > > pte_writable().
> > >
> > > You *have* to somehow rectify this situation. We can absolutely
> > > no
> > > leave pte_write() in its current, ambiguous state where it has
> > > no real
> > > meaning or where it is used to mean _both_ things depending on
> > > context.
> > True, the processor can always write to a page through a shadow
> > stack
> > PTE, but it must do that with a CALL instruction. Can we define
> > a
> > write operation as: MOV r1, *(r2). Then we don't have any doubt
> > on
> > pte_write() any more.
> No, we can't just move the target. :)
>
> You can define it this way, but then you also need to go to every
> spot
> in the kernel that calls pte_write() (and _PAGE_RW in fact) and
> audit it
> to ensure it means "mov ..." and not push.
Which pte_write() do you think is right?
bool is_shstk_pte(pte) {
return (_PAGE_RW not set) &&
(_PAGE_DIRTY_HW set);
}
int pte_write_1(pte) {
return (_PAGE_RW set) && !is_shstk_pte(pte);
}
int pte_write_2(pte) {
return (_PAGE_RW set) || is_shstk_pte(pte);
}
Yu-cheng
^ permalink raw reply
* Re: [PATCH v3 2/7] mm, slab/slub: introduce kmalloc-reclaimable caches
From: Roman Gushchin @ 2018-07-19 18:16 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Michal Hocko,
Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-3-vbabka@suse.cz>
On Wed, Jul 18, 2018 at 03:36:15PM +0200, Vlastimil Babka wrote:
> Kmem caches can be created with a SLAB_RECLAIM_ACCOUNT flag, which indicates
> they contain objects which can be reclaimed under memory pressure (typically
> through a shrinker). This makes the slab pages accounted as NR_SLAB_RECLAIMABLE
> in vmstat, which is reflected also the MemAvailable meminfo counter and in
> overcommit decisions. The slab pages are also allocated with __GFP_RECLAIMABLE,
> which is good for anti-fragmentation through grouping pages by mobility.
>
> The generic kmalloc-X caches are created without this flag, but sometimes are
> used also for objects that can be reclaimed, which due to varying size cannot
> have a dedicated kmem cache with SLAB_RECLAIM_ACCOUNT flag. A prominent example
> are dcache external names, which prompted the creation of a new, manually
> managed vmstat counter NR_INDIRECTLY_RECLAIMABLE_BYTES in commit f1782c9bc547
> ("dcache: account external names as indirectly reclaimable memory").
>
> To better handle this and any other similar cases, this patch introduces
> SLAB_RECLAIM_ACCOUNT variants of kmalloc caches, named kmalloc-rcl-X.
> They are used whenever the kmalloc() call passes __GFP_RECLAIMABLE among gfp
> flags. They are added to the kmalloc_caches array as a new type. Allocations
> with both __GFP_DMA and __GFP_RECLAIMABLE will use a dma type cache.
>
> This change only applies to SLAB and SLUB, not SLOB. This is fine, since SLOB's
> target are tiny system and this patch does add some overhead of kmem management
> objects.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> include/linux/slab.h | 16 +++++++++++----
> mm/slab_common.c | 48 ++++++++++++++++++++++++++++----------------
> 2 files changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 4299c59353a1..d89e934e0d8b 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -296,11 +296,12 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
> (KMALLOC_MIN_SIZE) : 16)
>
> #define KMALLOC_NORMAL 0
> +#define KMALLOC_RECLAIM 1
> #ifdef CONFIG_ZONE_DMA
> -#define KMALLOC_DMA 1
> -#define KMALLOC_TYPES 2
> +#define KMALLOC_DMA 2
> +#define KMALLOC_TYPES 3
> #else
> -#define KMALLOC_TYPES 1
> +#define KMALLOC_TYPES 2
> #endif
>
> #ifndef CONFIG_SLOB
> @@ -309,12 +310,19 @@ extern struct kmem_cache *kmalloc_caches[KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
> static __always_inline unsigned int kmalloc_type(gfp_t flags)
> {
> int is_dma = 0;
> + int is_reclaimable;
>
> #ifdef CONFIG_ZONE_DMA
> is_dma = !!(flags & __GFP_DMA);
> #endif
>
> - return is_dma;
> + is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
> +
> + /*
> + * If an allocation is botth __GFP_DMA and __GFP_RECLAIMABLE, return
^^
typo
> + * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
> + */
> + return (is_dma * 2) + (is_reclaimable & !is_dma);
Maybe
is_dma * KMALLOC_DMA + (is_reclaimable && !is_dma) * KMALLOC_RECLAIM
looks better?
> }
>
> /*
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 4614248ca381..614fb7ab8312 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1107,10 +1107,21 @@ void __init setup_kmalloc_cache_index_table(void)
> }
> }
>
> -static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
> +static void __init
> +new_kmalloc_cache(int idx, int type, slab_flags_t flags)
> {
> - kmalloc_caches[KMALLOC_NORMAL][idx] = create_kmalloc_cache(
> - kmalloc_info[idx].name,
> + const char *name;
> +
> + if (type == KMALLOC_RECLAIM) {
> + flags |= SLAB_RECLAIM_ACCOUNT;
> + name = kasprintf(GFP_NOWAIT, "kmalloc-rcl-%u",
> + kmalloc_info[idx].size);
> + BUG_ON(!name);
I'd replace this with WARN_ON() and falling back to kmalloc_info[idx].name.
> + } else {
> + name = kmalloc_info[idx].name;
> + }
> +
> + kmalloc_caches[type][idx] = create_kmalloc_cache(name,
> kmalloc_info[idx].size, flags, 0,
> kmalloc_info[idx].size);
> }
^ permalink raw reply
* Re: [RFC PATCH v2 16/27] mm: Modify can_follow_write_pte/pmd for shadow stack
From: Dave Hansen @ 2018-07-19 19:31 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <1532019963.16711.61.camel@intel.com>
On 07/19/2018 10:06 AM, Yu-cheng Yu wrote:
> Which pte_write() do you think is right?
There isn't one that's right.
The problem is that the behavior right now is ambiguous. Some callers
of pte_write() need to know about _PAGE_RW alone and others want to know
if (_PAGE_RW || is_shstk()).
The point is that you need both, plus a big audit of all the pte_write()
users to ensure they use the right one.
For instance, see spurious_fault_check(). We can get a shadowstack
fault that also has X86_PF_WRITE, but pte_write()==0. That might make a
shadowstack write fault falsely appear spurious.
^ permalink raw reply
* Re: [PATCH v3 0/7] kmalloc-reclaimable caches
From: Roman Gushchin @ 2018-07-19 19:53 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Michal Hocko,
Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
Mel Gorman, Matthew Wilcox, Laura Abbott, Sumit Semwal,
Vijayanand Jitta
In-Reply-To: <20180718133620.6205-1-vbabka@suse.cz>
On Wed, Jul 18, 2018 at 03:36:13PM +0200, Vlastimil Babka wrote:
> v3 changes:
> - fix missing hunk in patch 5/7
> - more verbose cover letter and patch 6/7 commit log
> v2 changes:
> - shorten cache names to kmalloc-rcl-<SIZE>
> - last patch shortens <SIZE> for all kmalloc caches to e.g. "1k", "4M"
> - include dma caches to the 2D kmalloc_caches[] array to avoid a branch
> - vmstat counter nr_indirectly_reclaimable_bytes renamed to
> nr_kernel_misc_reclaimable, doesn't include kmalloc-rcl-*
> - /proc/meminfo counter renamed to KReclaimable, includes kmalloc-rcl*
> and nr_kernel_misc_reclaimable
>
> Hi,
>
> as discussed at LSF/MM [1] here's a patchset that introduces
> kmalloc-reclaimable caches (more details in the second patch) and uses them for
> SLAB freelists and dcache external names. The latter allows us to repurpose the
> NR_INDIRECTLY_RECLAIMABLE_BYTES counter later in the series.
>
> With patch 4/7, dcache external names are allocated from kmalloc-rcl-*
> caches, eliminating the need for manual accounting. More importantly, it
> also ensures the reclaimable kmalloc allocations are grouped in pages
> separate from the regular kmalloc allocations. The need for proper
> accounting of dcache external names has shown it's easy for misbehaving
> process to allocate lots of them, causing premature OOMs. Without the
> added grouping, it's likely that a similar workload can interleave the
> dcache external names allocations with regular kmalloc allocations
> (note: I haven't searched myself for an example of such regular kmalloc
> allocation, but I would be very surprised if there wasn't some). A
> pathological case would be e.g. one 64byte regular allocations with 63
> external dcache names in a page (64x64=4096), which means the page is
> not freed even after reclaiming after all dcache names, and the process
> can thus "steal" the whole page with single 64byte allocation.
>
> If there other kmalloc users similar to dcache external names become
> identified, they can also benefit from the new functionality simply by
> adding __GFP_RECLAIMABLE to the kmalloc calls.
>
> Side benefits of the patchset (that could be also merged separately)
> include removed branch for detecting __GFP_DMA kmalloc(), and shortening
> kmalloc cache names in /proc/slabinfo output. The latter is potentially
> an ABI break in case there are tools parsing the names and expecting the
> values to be in bytes.
>
> This is how /proc/slabinfo looks like after booting in virtme:
>
> ...
> kmalloc-rcl-4M 0 0 4194304 1 1024 : tunables 1 1 0 : slabdata 0 0 0
> ...
> kmalloc-rcl-96 7 32 128 32 1 : tunables 120 60 8 : slabdata 1 1 0
> kmalloc-rcl-64 25 128 64 64 1 : tunables 120 60 8 : slabdata 2 2 0
> kmalloc-rcl-32 0 0 32 124 1 : tunables 120 60 8 : slabdata 0 0 0
> kmalloc-4M 0 0 4194304 1 1024 : tunables 1 1 0 : slabdata 0 0 0
> kmalloc-2M 0 0 2097152 1 512 : tunables 1 1 0 : slabdata 0 0 0
> kmalloc-1M 0 0 1048576 1 256 : tunables 1 1 0 : slabdata 0 0 0
> ...
>
> /proc/vmstat with renamed nr_indirectly_reclaimable_bytes counter:
>
> ...
> nr_slab_reclaimable 2817
> nr_slab_unreclaimable 1781
> ...
> nr_kernel_misc_reclaimable 0
> ...
>
> /proc/meminfo with new KReclaimable counter:
>
> ...
> Shmem: 564 kB
> KReclaimable: 11260 kB
> Slab: 18368 kB
> SReclaimable: 11260 kB
> SUnreclaim: 7108 kB
> KernelStack: 1248 kB
> ...
>
> Thanks,
> Vlastimil
Hi, Vlastimil!
Overall the patchset looks solid to me.
Please, feel free to add
Acked-by: Roman Gushchin <guro@fb.com>
Two small nits:
1) The last patch is unrelated to the main idea,
and can potentially cause ABI breakage.
I'd separate it from the rest of the patchset.
2) It's actually re-opening the security issue for SLOB
users. Is the memory overhead really big enough to
justify that?
Thanks!
^ permalink raw reply
* Re: [PATCH v3 1/7] mm, slab: combine kmalloc_caches and kmalloc_dma_caches
From: Vlastimil Babka @ 2018-07-20 9:30 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, Christoph Lameter, David Rientjes,
Joonsoo Kim, Matthew Wilcox, Luis R. Rodriguez
In-Reply-To: <20180719081020.5pl3naynwhgev6rx@techsingularity.net>
On 07/19/2018 10:10 AM, Mel Gorman wrote:
> On Wed, Jul 18, 2018 at 03:36:14PM +0200, Vlastimil Babka wrote:
>> The kmalloc caches currently mainain separate (optional) array
>> kmalloc_dma_caches for __GFP_DMA allocations. There are tests for __GFP_DMA in
>> the allocation hotpaths. We can avoid the branches by combining kmalloc_caches
>> and kmalloc_dma_caches into a single two-dimensional array where the outer
>> dimension is cache "type". This will also allow to add kmalloc-reclaimable
>> caches as a third type.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>
> I'm surprised there are so many kmalloc users that require the DMA zone.
> Some of them are certainly bogus such as in drivers for archs that only
> have one zone and is probably a reflection of the confusing naming. The
> audit would be a mess and unrelated to the patch so for this patch;
Yeah, there was a session about that on LSF/MM and Luis was working on
it. One of the motivations was to get rid of the branch, so that's
sidestepped by this patch. I would still like to not have slabinfo full
of empty dma-kmalloc caches though :)
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
>
^ permalink raw reply
* Re: [PATCH v3 2/7] mm, slab/slub: introduce kmalloc-reclaimable caches
From: Vlastimil Babka @ 2018-07-20 9:32 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, Christoph Lameter, David Rientjes,
Joonsoo Kim, Matthew Wilcox
In-Reply-To: <20180719082319.6jkltwinon3pyzyn@techsingularity.net>
On 07/19/2018 10:23 AM, Mel Gorman wrote:
>> /*
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 4614248ca381..614fb7ab8312 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -1107,10 +1107,21 @@ void __init setup_kmalloc_cache_index_table(void)
>> }
>> }
>>
>> -static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
>> +static void __init
>> +new_kmalloc_cache(int idx, int type, slab_flags_t flags)
>> {
>> - kmalloc_caches[KMALLOC_NORMAL][idx] = create_kmalloc_cache(
>> - kmalloc_info[idx].name,
>> + const char *name;
>> +
>> + if (type == KMALLOC_RECLAIM) {
>> + flags |= SLAB_RECLAIM_ACCOUNT;
>> + name = kasprintf(GFP_NOWAIT, "kmalloc-rcl-%u",
>> + kmalloc_info[idx].size);
>> + BUG_ON(!name);
>> + } else {
>> + name = kmalloc_info[idx].name;
>> + }
>> +
>> + kmalloc_caches[type][idx] = create_kmalloc_cache(name,
>> kmalloc_info[idx].size, flags, 0,
>> kmalloc_info[idx].size);
>> }
>
> I was going to query that BUG_ON but if I'm reading it right, we just
> have to be careful in the future that the "normal" kmalloc cache is always
> initialised before the reclaimable cache or there will be issues.
Yeah, I was just copying how the dma-kmalloc code does it.
>> @@ -1122,22 +1133,25 @@ static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
>> */
>> void __init create_kmalloc_caches(slab_flags_t flags)
>> {
>> - int i;
>> - int type = KMALLOC_NORMAL;
>> + int i, type;
>>
>> - for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
>> - if (!kmalloc_caches[type][i])
>> - new_kmalloc_cache(i, flags);
>> + for (type = KMALLOC_NORMAL; type <= KMALLOC_RECLAIM; type++) {
>> + for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
>> + if (!kmalloc_caches[type][i])
>> + new_kmalloc_cache(i, type, flags);
>>
>
> I don't see a problem here as such but the values of the KMALLOC_* types
> is important both for this function and the kmalloc_type(). It might be
> worth adding a warning that these functions be examined if updating the
> types but then again, anyone trying and getting it wrong will have a
> broken kernel so;
OK
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
Thanks!
^ permalink raw reply
* Re: [PATCH v3 2/7] mm, slab/slub: introduce kmalloc-reclaimable caches
From: Vlastimil Babka @ 2018-07-20 9:35 UTC (permalink / raw)
To: Roman Gushchin
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Michal Hocko,
Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
Mel Gorman, Matthew Wilcox
In-Reply-To: <20180719181613.GA26595@castle.DHCP.thefacebook.com>
On 07/19/2018 08:16 PM, Roman Gushchin wrote:
>> is_dma = !!(flags & __GFP_DMA);
>> #endif
>>
>> - return is_dma;
>> + is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
>> +
>> + /*
>> + * If an allocation is botth __GFP_DMA and __GFP_RECLAIMABLE, return
> ^^
> typo
>> + * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
>> + */
>> + return (is_dma * 2) + (is_reclaimable & !is_dma);
>
> Maybe
> is_dma * KMALLOC_DMA + (is_reclaimable && !is_dma) * KMALLOC_RECLAIM
> looks better?
I think I meant to do that but forgot, thanks.
>> }
>>
>> /*
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 4614248ca381..614fb7ab8312 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -1107,10 +1107,21 @@ void __init setup_kmalloc_cache_index_table(void)
>> }
>> }
>>
>> -static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
>> +static void __init
>> +new_kmalloc_cache(int idx, int type, slab_flags_t flags)
>> {
>> - kmalloc_caches[KMALLOC_NORMAL][idx] = create_kmalloc_cache(
>> - kmalloc_info[idx].name,
>> + const char *name;
>> +
>> + if (type == KMALLOC_RECLAIM) {
>> + flags |= SLAB_RECLAIM_ACCOUNT;
>> + name = kasprintf(GFP_NOWAIT, "kmalloc-rcl-%u",
>> + kmalloc_info[idx].size);
>> + BUG_ON(!name);
>
> I'd replace this with WARN_ON() and falling back to kmalloc_info[idx].name.
It's basically a copy/paste of the dma-kmalloc code. If that triggers,
it means somebody was changing the code and introduced a wrong order (as
Mel said). A system that genuinely has no memory for that printf at this
point, would not get very far anyway...
>> + } else {
>> + name = kmalloc_info[idx].name;
>> + }
>> +
>> + kmalloc_caches[type][idx] = create_kmalloc_cache(name,
>> kmalloc_info[idx].size, flags, 0,
>> kmalloc_info[idx].size);
>> }
^ permalink raw reply
* Re: [PATCH v3 3/7] mm, slab: allocate off-slab freelists as reclaimable when appropriate
From: Vlastimil Babka @ 2018-07-20 9:37 UTC (permalink / raw)
To: Mel Gorman
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, Christoph Lameter, David Rientjes,
Joonsoo Kim, Matthew Wilcox
In-Reply-To: <20180719083530.jhugqzkvjnbrddim@techsingularity.net>
On 07/19/2018 10:35 AM, Mel Gorman wrote:
> On Wed, Jul 18, 2018 at 03:36:16PM +0200, Vlastimil Babka wrote:
>> In SLAB, OFF_SLAB caches allocate management structures (currently just the
>> freelist) from kmalloc caches when placement in a slab page together with
>> objects would lead to suboptimal memory usage. For SLAB_RECLAIM_ACCOUNT caches,
>> we can allocate the freelists from the newly introduced reclaimable kmalloc
>> caches, because shrinking the OFF_SLAB cache will in general result to freeing
>> of the freelists as well. This should improve accounting and anti-fragmentation
>> a bit.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>
> I'm not quite convinced by this one. The freelist cache is tied to the
> lifetime of the slab and not the objects. A single freelist can be reclaimed
> eventually but for caches with many objects per slab, it could take a lot
> of shrinking random objects to reclaim one freelist. Functionally the
> patch appears to be fine.
Hm you're right that the reclaimability of freelist is maybe too much
detached, and could do more harm than good for the reclaimable caches. I
will probably drop it unless I can measure it's an improvement. Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/7] kmalloc-reclaimable caches
From: Vlastimil Babka @ 2018-07-20 9:45 UTC (permalink / raw)
To: Roman Gushchin
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Michal Hocko,
Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
Mel Gorman, Matthew Wilcox, Laura Abbott, Sumit Semwal,
Vijayanand Jitta
In-Reply-To: <20180719195332.GB26595@castle.DHCP.thefacebook.com>
On 07/19/2018 09:53 PM, Roman Gushchin wrote:
>> Vlastimil
> Overall the patchset looks solid to me.
> Please, feel free to add
> Acked-by: Roman Gushchin <guro@fb.com>
Thanks!
> Two small nits:
> 1) The last patch is unrelated to the main idea,
> and can potentially cause ABI breakage.
Yes, that's why it's last.
> I'd separate it from the rest of the patchset.
It's not independent though because there would be conflicts. It has to
be decided if it goes before of after the rest. Putting it last in the
series makes the order clear and makes it possible to revert it in case
it does break any users, without disrupting the rest of the series.
> 2) It's actually re-opening the security issue for SLOB
> users. Is the memory overhead really big enough to
> justify that?
I assume that anyone choosing SLOB has a tiny embedded device which runs
only pre-flashed code, so that's less of an issue. If somebody can
trigger the issue remotely, there are likely also other ways to exhaust
the limited memory there?
> Thanks!
^ permalink raw reply
* Re: [RFC PATCH v2 14/27] mm: Handle THP/HugeTLB shadow stack page fault
From: Dave Hansen @ 2018-07-20 14:20 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <20180710222639.8241-15-yu-cheng.yu@intel.com>
On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> @@ -1193,6 +1195,8 @@ static int do_huge_pmd_wp_page_fallback(struct vm_fault *vmf, pmd_t orig_pmd,
> pte_t entry;
> entry = mk_pte(pages[i], vma->vm_page_prot);
> entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> + if (is_shstk_mapping(vma->vm_flags))
> + entry = pte_mkdirty_shstk(entry);
Peter Z was pointing out that we should get rid of all this generic code
manipulation. We might not easily be able to do it *all*, but we can do
better than what we've got here.
Basically, if you have code outside of arch/x86 in your patch set that
refers to shadow stacks, you should consider it a bug (for now),
especially if you have to hack .c files.
For instance, in the code above, you could move the is_shstk_mapping() into:
static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
{
if (likely(vma->vm_flags & VM_WRITE))
pte = pte_mkwrite(pte);
+ pte = arch_pte_mkwrite(pte, vma);
+
return pte;
}
... and add an arch callback that does:
static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
{
if (!is_shstk_mapping(vma->vm_flags))
return pte;
WARN_ON(... pte bits incompatible with shadow stacks?);
/* Lots of comments of course */
entry = pte_mkdirty_shstk(entry);
}
This is just one example. You are probably going to need a couple of
similar things. Just remember: the bar is very high to make changes to
.c files outside of arch/x86. You can do a _bit_ more in non-x86
headers, but you have the most freedom to patch what you want as long as
it's in arch/x86.
^ permalink raw reply
* Re: [RFC PATCH v2 14/27] mm: Handle THP/HugeTLB shadow stack page fault
From: Yu-cheng Yu @ 2018-07-20 14:58 UTC (permalink / raw)
To: Dave Hansen, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <adc8f0b0-e2a5-d3e9-edaf-8d5b3be6a5b6@linux.intel.com>
On Fri, 2018-07-20 at 07:20 -0700, Dave Hansen wrote:
> On 07/10/2018 03:26 PM, Yu-cheng Yu wrote:
> >
> > @@ -1193,6 +1195,8 @@ static int
> > do_huge_pmd_wp_page_fallback(struct vm_fault *vmf, pmd_t orig_pmd,
> > pte_t entry;
> > entry = mk_pte(pages[i], vma->vm_page_prot);
> > entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> > + if (is_shstk_mapping(vma->vm_flags))
> > + entry = pte_mkdirty_shstk(entry);
> Peter Z was pointing out that we should get rid of all this generic
> code
> manipulation. We might not easily be able to do it *all*, but we
> can do
> better than what we've got here.
>
> Basically, if you have code outside of arch/x86 in your patch set
> that
> refers to shadow stacks, you should consider it a bug (for now),
> especially if you have to hack .c files.
>
> For instance, in the code above, you could move the
> is_shstk_mapping() into:
>
> static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct
> *vma)
> {
> if (likely(vma->vm_flags & VM_WRITE))
> pte = pte_mkwrite(pte);
>
> + pte = arch_pte_mkwrite(pte, vma);
> +
> return pte;
> }
>
> ... and add an arch callback that does:
>
> static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct
> *vma)
> {
> if (!is_shstk_mapping(vma->vm_flags))
> return pte;
>
> WARN_ON(... pte bits incompatible with shadow stacks?);
>
> /* Lots of comments of course */
> entry = pte_mkdirty_shstk(entry);
> }
>
> This is just one example. You are probably going to need a couple
> of
> similar things. Just remember: the bar is very high to make changes
> to
> .c files outside of arch/x86. You can do a _bit_ more in non-x86
> headers, but you have the most freedom to patch what you want as
> long as
> it's in arch/x86.
Ok, I will work on that. Thanks!
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <0377c3ced6bdbc44fe17f9a5679cb6eda4304024.1528304203.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement the proc fs write to set the audit container identifier of a
> process, emitting an AUDIT_CONTAINER_ID record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/audit_containerid where PID is the process ID of the
> newly created task that is to become the first task in a container, or
> an additional task added to a container.
>
> The write expects up to a u64 value (unset: 18446744073709551615).
>
> The writer must have capability CAP_AUDIT_CONTROL.
>
> This will produce a record such as this:
> type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
>
> The "op" field indicates an initial set. The "pid" to "ses" fields are
> the orchestrator while the "opid" field is the object's PID, the process
> being "contained". Old and new audit container identifier values are
> given in the "contid" fields, while res indicates its success.
>
> It is not permitted to unset or re-set the audit container identifier.
> A child inherits its parent's audit container identifier, but then can
> be set only once after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/proc/base.c | 37 ++++++++++++++++++++++++
> include/linux/audit.h | 25 ++++++++++++++++
> include/uapi/linux/audit.h | 2 ++
> kernel/auditsc.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 135 insertions(+)
...
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -606,6 +621,16 @@ static inline bool audit_loginuid_set(struct task_struct *tsk)
> return uid_valid(audit_get_loginuid(tsk));
> }
>
> +static inline bool cid_valid(u64 contid)
> +{
> + return contid != AUDIT_CID_UNSET;
> +}
> +
> +static inline bool audit_contid_set(struct task_struct *tsk)
> +{
> + return cid_valid(audit_get_contid(tsk));
> +}
For the sake of consistency I think we should rename cid_valid() to
audit_contid_valid().
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 59ef7a81..611e926 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -956,6 +956,8 @@ int audit_alloc(struct task_struct *tsk)
> return -ENOMEM;
> info->loginuid = audit_get_loginuid(current);
> info->sessionid = audit_get_sessionid(current);
> + info->contid = audit_get_contid(current);
> + info->inherited = true;
First see my others comments in this patch about inheritence, but if
we decide that flagging inherited values is important we should
probably rename the "inherited" field to indicate that it applies to
just the "contid" field.
> tsk->audit = info;
>
> if (likely(!audit_ever_enabled))
> @@ -985,6 +987,8 @@ int audit_alloc(struct task_struct *tsk)
> struct audit_task_info init_struct_audit = {
> .loginuid = INVALID_UID,
> .sessionid = AUDIT_SID_UNSET,
> + .contid = AUDIT_CID_UNSET,
> + .inherited = true,
> .ctx = NULL,
> };
>
> @@ -2112,6 +2116,73 @@ int audit_set_loginuid(kuid_t loginuid)
> }
>
> /**
> + * audit_set_contid - set current task's audit_context contid
> + * @contid: contid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_contid_write().
> + */
> +int audit_set_contid(struct task_struct *task, u64 contid)
> +{
> + u64 oldcontid;
> + int rc = 0;
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> + char comm[sizeof(current->comm)];
> +
> + /* Can't set if audit disabled */
> + if (!task->audit)
> + return -ENOPROTOOPT;
> + oldcontid = audit_get_contid(task);
> + /* Don't allow the audit containerid to be unset */
> + if (!cid_valid(contid))
> + rc = -EINVAL;
> + /* if we don't have caps, reject */
> + else if (!capable(CAP_AUDIT_CONTROL))
> + rc = -EPERM;
> + /* if task has children or is not single-threaded, deny */
> + else if (!list_empty(&task->children))
> + rc = -EBUSY;
Is this safe without holding tasklist_lock? I worry we might be
vulnerable to a race with fork().
> + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> + rc = -EALREADY;
Similar concern here as well, although related to threads.
> + /* it is already set, and not inherited from the parent, reject */
> + else if (cid_valid(oldcontid) && !task->audit->inherited)
> + rc = -EEXIST;
Maybe I'm missing something, but why do we care about preventing
reassigning the audit container ID in this case? The task is single
threaded and has no descendants at this point so it should be safe,
yes? So long as the task changing the audit container ID has
capable(CAP_AUDIT_CONTOL) it shouldn't matter, right?
Related, I'm questioning if we would ever care if the audit container
ID was inherited or not?
> + if (!rc) {
> + task_lock(task);
> + task->audit->contid = contid;
> + task->audit->inherited = false;
> + task_unlock(task);
I suspect the task_lock() may not be what we want here, but if we are
using task_lock() to protect the audit fields two things come to mind:
1. We should update the header comments for task_lock() in task.h to
indicate that it also protects ->audit.
2. Where else do we need to worry about taking this lock? At the very
least we should take this lock near the top of this function before we
check task->audit and not drop it until after we have set it, or
failed the operation for one of the reasons above.
> + }
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_ID);
> + if (!ab)
> + return rc;
> +
> + uid = from_kuid(&init_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> + task_tgid_nr(task), oldcontid, contid,
> + task_tgid_nr(current), uid
> + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> + tty ? tty_name(tty) : "(none)",
> + audit_get_sessionid(current));
> + audit_put_tty(tty);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " comm=");
> + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> + audit_log_d_path_exe(ab, current->mm);
> + audit_log_format(ab, " res=%d", !rc);
> + audit_log_end(ab);
> + return rc;
> +}
> +
> +/**
> * __audit_mq_open - record audit data for a POSIX MQ open
> * @oflag: open flag
> * @mode: mode bits
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 02/10] audit: log container info of syscalls
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <b839dc7fb29aae226b2a4602e7ef3c2a406ee90a.1528304203.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Create a new audit record AUDIT_CONTAINER to document the audit
> container identifier of a process if it is present.
>
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 7 +++++++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 23 +++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 4 files changed, 34 insertions(+)
...
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
> #define AUDIT_REPLACE 1329 /* Replace auditd if this packet unanswerd */
> #define AUDIT_KERN_MODULE 1330 /* Kernel Module events */
> #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */
> +#define AUDIT_CONTAINER 1332 /* Container ID */
I'm not sure I'm completely sold on the AUDIT_CONTAINER_ID and
AUDIT_CONTAINER record type names. From what I can tell
AUDIT_CONTAINER_ID seems to be used for audit container ID management
operations, e.g. setting the ID, whereas the AUDIT_CONTAINER is used
to tag events with the corresponding audit container ID. Assuming
that is correct, it seems like AUDIT_CONTAINER might be better served
if it was named AUDIT_CONTAINER_ID and if we could change
AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc. Thoughts?
> #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb..5e150c6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
> audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> }
>
> +/*
> + * audit_log_contid - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + * @op: contid string description
> + */
> +int audit_log_contid(struct task_struct *tsk,
> + struct audit_context *context, char *op)
> +{
> + struct audit_buffer *ab;
> +
> + if (!audit_contid_set(tsk))
> + return 0;
> + /* Generate AUDIT_CONTAINER record with container ID */
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return -ENOMEM;
> + audit_log_format(ab, "op=%s contid=%llu",
> + op, audit_get_contid(tsk));
Can you explain your reason for including an "op" field in this record
type? I've been looking at the rest of the patches in this patchset
and it seems to be used more as an indicator of the record's
generating context rather than any sort of audit container ID
operation.
> + audit_log_end(ab);
> + return 0;
> +}
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 03/10] audit: add containerid support for ptrace and signals
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <29359e0e6bc34c74b3a2c3ce0cdfda77f530cf18.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier support to ptrace and signals. In
> particular, the "op" field provides a way to label the auxiliary record
> to which it is associated.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 11 +++++------
> kernel/audit.c | 13 +++++++------
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 21 ++++++++++++++++-----
> 4 files changed, 30 insertions(+), 17 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 5e150c6..ba304a8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -142,6 +142,7 @@ struct audit_net {
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> u32 audit_sig_sid = 0;
> +u64 audit_sig_cid = AUDIT_CID_UNSET;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1437,6 +1438,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> + sig_data->cid = audit_sig_cid;
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> kfree(sig_data);
> @@ -2050,23 +2052,22 @@ void audit_log_session_info(struct audit_buffer *ab)
>
> /*
> * audit_log_contid - report container info
> - * @tsk: task to be recorded
> * @context: task or local context for record
> * @op: contid string description
> + * @contid: container ID to report
> */
> -int audit_log_contid(struct task_struct *tsk,
> - struct audit_context *context, char *op)
> +int audit_log_contid(struct audit_context *context,
> + char *op, u64 contid)
> {
> struct audit_buffer *ab;
>
> - if (!audit_contid_set(tsk))
> + if (!cid_valid(contid))
> return 0;
> /* Generate AUDIT_CONTAINER record with container ID */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> if (!ab)
> return -ENOMEM;
> - audit_log_format(ab, "op=%s contid=%llu",
> - op, audit_get_contid(tsk));
> + audit_log_format(ab, "op=%s contid=%llu", op, contid);
> audit_log_end(ab);
> return 0;
> }
You might as well just make these changes to audit_log_contid() in
patch 2 when you first define audit_log_contid().
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 04/10] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <dcd3d6e27c4beb8aec11805faeb4de7013132bd7.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 8 ++++++++
> kernel/auditsc.c | 25 +++++++++++++++++++++++--
> 2 files changed, 31 insertions(+), 2 deletions(-)
...
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -916,8 +916,11 @@ static inline void audit_free_aux(struct audit_context *context)
> static inline struct audit_context *audit_alloc_context(enum audit_state state)
> {
> struct audit_context *context;
> + gfp_t gfpflags;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
> + gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
Instead of trying to guess the proper gfp flags, and likely getting it
wrong at some point (the in_atomic() comment in preempt.h don't give
me the warm fuzzies), why not pass the gfp flags as an argument?
Right now it looks like we would only have two callers: audit_alloc()
and audit_audit_local(). The audit_alloc() invocation would simply
pass GFP_KERNEL and we could allow the audit_alloc_local() callers to
specify the gfp flags when calling audit_alloc_local() (although I
suspect that will always be GFP_ATOMIC since we should only be calling
audit_alloc_local() from interrupt-like context, in all other cases we
should use the audit_context from the current task_struct.
> + context = kzalloc(sizeof(*context), gfpflags);
> if (!context)
> return NULL;
> context->state = state;
> @@ -993,8 +996,26 @@ struct audit_task_info init_struct_audit = {
> .ctx = NULL,
> };
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
> {
Let's see where this goes, but we may want to rename this slightly to
indicate that this should only be called from interrupt context when
we can't rely on current's audit_context. Bonus points if we can find
a way to enforce this with a WARN() assertion so we can better catch
abuse.
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->in_syscall = 1;
Setting in_syscall is both interesting and a bit troubling, if for no
other reason than I expect most (all?) callers to be in an interrupt
context when audit_alloc_local() is called. Setting in_syscall would
appear to be conceptually in this case. Can you help explain why this
is the right thing to do, or necessary to ensure things are handled
correctly?
Looking quickly at the audit code, it seems to only be used on record
and/or syscall termination to end things properly as well as in some
of the PATH record code paths to limit filename collection to actual
syscalls. However, this was just a quick look so I could be missing
some important points.
> + return context;
> +}
> +
> +void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 05/10] audit: add containerid support for tty_audit
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <28ab8ad3c4e5de6f61b928eeb2af030b04a8820b.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:04 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record to tty logging rule
> event standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> drivers/tty/tty_audit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> index e30aa6b..66bd850 100644
> --- a/drivers/tty/tty_audit.c
> +++ b/drivers/tty/tty_audit.c
> @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
> uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
> unsigned int sessionid = audit_get_sessionid(tsk);
> + struct audit_context *context = audit_alloc_local();
We should be using current's audit_context in tty_audit_log().
Actually, we should probably just get rid of the tsk variable in
tty_audit_log() and use current directly to make things a bit more
obvious.
<time passes>
I did some digging and I have a two year old, half-baked patch that
cleans up this tsk/current usage as well as a few others. I just
rebased it against audit/next and surprisingly it seems to pass a
basic smoke test (kernel boots and audit-testsuite passes); I'll post
it to the list as a RFC once I'm done reviewing these patches.
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> if (ab) {
> char name[sizeof(tsk->comm)];
>
> @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> audit_log_n_hex(ab, data, size);
> audit_log_end(ab);
> }
> + audit_log_contid(context, "tty", audit_get_contid(tsk));
> + audit_free_context(context);
> }
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 06/10] audit: add containerid filtering
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <cc924bc23937ea3d65b4b2b625796ee61a5fb4da.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement audit container identifier filtering using the AUDIT_CONTID
> field name to send an 8-character string representing a u64 since the
> value field is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
>
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER.
We've had some discussions about starting to be a bit more "stingy"
with our feature bitmap fields, and normally I'm not sure I would want
to burn one on a new filter (we should be able to fail safely here),
but considering the significance of the audit container ID work I
think it might be okay to burn a bit :)
How about renaming this to just AUDIT_FEATURE_BITMAP_CONTAINERID?
> See: https://github.com/linux-audit/audit-kernel/issues/91
> See: https://github.com/linux-audit/audit-userspace/issues/40
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 ++++-
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f549121..1e37abf 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -76,6 +76,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> + u64 val64;
> kuid_t uid;
> kgid_t gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 469ab25..b440558 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -262,6 +262,7 @@
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> #define AUDIT_FSTYPE 26 /* FileSystem Type */
> +#define AUDIT_CONTID 27 /* Container ID */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -342,6 +343,7 @@ enum {
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER 0x00000080
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -349,7 +351,8 @@ enum {
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 1cf1c35..743d445 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -235,6 +235,7 @@ static inline int audit_hash_ino(u32 ino)
>
> extern int audit_match_class(int class, unsigned syscall);
> extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index eaa3201..a5f60ce 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_CONTID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> + case AUDIT_CONTID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -666,6 +675,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp, audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1208,6 +1226,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1346,6 +1389,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result = audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op, f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 81c9765..ea1ee35 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -622,6 +622,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 07/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <562cfaf7629b64252aac7cf3cecdd70b471af5b5.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task. The network
> namespace could in use by multiple containers by association to the
> tasks in that network namespace. We still want a way to attribute
> these events to any potential containers. Keep a list per network
> namespace to track these audit container identifiiers.
>
> Add/increment the audit container identifier on:
> - initial setting of the audit container identifier via /proc
> - clone/fork call that inherits an audit container identifier
> - unshare call that inherits an audit container identifier
> - setns call that inherits an audit container identifier
> Delete/decrement the audit container identifier on:
> - an inherited audit container identifier dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/92
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 23 ++++++++++++++++
> kernel/audit.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 5 ++++
> kernel/nsproxy.c | 4 +++
> 4 files changed, 104 insertions(+)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e37abf..7e2e51c 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -87,6 +88,12 @@ struct audit_field {
> u32 op;
> };
>
> +struct audit_contid {
> + struct list_head list;
> + u64 id;
> + refcount_t refcount;
> +};
Do we need to worry about locking the audit container ID list? Does
the network namespace code (or some other namespace code) ensure that
add/deletes are serialized?
> @@ -156,6 +163,10 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> struct task_struct *tsk);
> extern int audit_log_contid(struct audit_context *context,
> char *op, u64 contid);
> +extern struct list_head *audit_get_contid_list(const struct net *net);
> +extern void audit_contid_add(struct net *net, u64 contid);
> +extern void audit_contid_del(struct net *net, u64 contid);
I wonder if we should change these function names to indicate that
they are managing the netns/cid list? Right now there is no mention
of networking other than the first parameter.
Maybe audit_netns_contid_*()?
> +extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
>
> extern int audit_update_lsm_rules(void);
>
> @@ -209,6 +220,18 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
> static inline int audit_log_contid(struct audit_context *context,
> char *op, u64 contid)
> { }
> +static inline struct list_head *audit_get_contid_list(const struct net *net)
> +{
> + static LIST_HEAD(list);
> + return &list;
> +}
Why can't we just return NULL here like a normal dummy function? It's
only ever used inside audit. Actually, why is this even in
include/linux/audit.h, couldn't we put it in kernel/audit.h or even
just make it a static in audit.c?
> +static inline void audit_contid_add(struct net *net, u64 contid)
> +{ }
> +static inline void audit_contid_del(struct net *net, u64 contid)
> +{ }
> +static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{ }
> +
> #define audit_enabled 0
> #endif /* CONFIG_AUDIT */
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ba304a8..ecd2de4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -106,6 +106,7 @@
> */
> struct audit_net {
> struct sock *sk;
> + struct list_head contid_list;
> };
>
> /**
> @@ -311,6 +312,76 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> +/**
> + * audit_get_contid_list - Return the audit container ID list for the given network namespace
> + * @net: the destination network namespace
> + *
> + * Description:
> + * Returns the list pointer if valid, NULL otherwise. The caller must ensure
> + * that a reference is held for the network namespace while the sock is in use.
> + */
> +struct list_head *audit_get_contid_list(const struct net *net)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> +
> + return &aunet->contid_list;
> +}
> +
> +void audit_contid_add(struct net *net, u64 contid)
> +{
> + struct list_head *contid_list = audit_get_contid_list(net);
> + struct audit_contid *cont;
> +
> + if (!cid_valid(contid))
> + return;
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + return;
> + }
<thinking out loud>I think this is fine for right now, but we may need
to be a bit clever about how we store the IDs - walking an unsorted
list with lots of entries may prove to be too painful.</thinking out
loud>
> + cont = kmalloc(sizeof(struct audit_contid), GFP_KERNEL);
> + if (!cont)
> + return;
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add(&cont->list, contid_list);
> +}
> +
> +void audit_contid_del(struct net *net, u64 contid)
> +{
> + struct list_head *contid_list = audit_get_contid_list(net);
> + struct audit_contid *cont = NULL;
> + int found = 0;
> +
> + if (!cid_valid(contid))
> + return;
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + found = 1;
> + break;
You don't really need the found variable, you can just move all of the
work you need to do up into the if statement and return from inside
the if statement.
> + }
> + if (!found)
> + return;
> + list_del(&cont->list);
> + if (refcount_dec_and_test(&cont->refcount))
> + kfree(cont);
Don't you want to dec_and_test first and only remove it from the list
if there are no other references?
> +}
>
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!cid_valid(contid))
> + return;
> + audit_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1550,6 +1621,7 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> + INIT_LIST_HEAD(&aunet->contid_list);
>
> return 0;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ea1ee35..6ab5e5e 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
> #include <linux/uaccess.h>
> #include <linux/fsnotify_backend.h>
> #include <uapi/linux/limits.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -2165,6 +2166,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> /* Can't set if audit disabled */
> if (!task->audit)
> @@ -2185,10 +2187,13 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (cid_valid(oldcontid) && !task->audit->inherited)
> rc = -EEXIST;
> if (!rc) {
> + if (cid_valid(oldcontid))
> + audit_contid_del(net, oldcontid);
> task_lock(task);
> task->audit->contid = contid;
> task->audit->inherited = false;
> task_unlock(task);
> + audit_contid_add(net, contid);
> }
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d33..dcb69fe 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include <linux/syscalls.h>
> #include <linux/cgroup.h>
> #include <linux/perf_event.h>
> +#include <linux/audit.h>
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 08/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Paul Moore @ 2018-07-20 22:15 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <c6b64f6540e2e531636142ced36d51f0744d0e1f.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 5 +++++
> kernel/audit.c | 20 +++++++++++++++++++-
> kernel/auditsc.c | 2 ++
> net/netfilter/xt_AUDIT.c | 12 ++++++++++--
> 4 files changed, 36 insertions(+), 3 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 7e2e51c..4560a4e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -167,6 +167,8 @@ extern int audit_log_contid(struct audit_context *context,
> extern void audit_contid_add(struct net *net, u64 contid);
> extern void audit_contid_del(struct net *net, u64 contid);
> extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
> +extern void audit_log_contid_list(struct net *net,
> + struct audit_context *context);
See my comment in previous patches about changing the function name to
better indicate it's dedicate use for network namespaces.
> extern int audit_update_lsm_rules(void);
>
> @@ -231,6 +233,9 @@ static inline void audit_contid_del(struct net *net, u64 contid)
> { }
> static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> { }
> +static inline void audit_log_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled 0
> #endif /* CONFIG_AUDIT */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ecd2de4..8cca41a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -382,6 +382,20 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_contid_add(new->net_ns, contid);
> }
>
> +void audit_log_contid_list(struct net *net, struct audit_context *context)
> +{
> + struct audit_contid *cont;
> + int i = 0;
> +
> + list_for_each_entry(cont, audit_get_contid_list(net), list) {
> + char buf[14];
> +
> + sprintf(buf, "net%u", i++);
> + audit_log_contid(context, buf, cont->id);
Hmm. It looks like this will generate multiple audit container ID
records with "op=netX contid=Y" (X=netns number, Y=audit container
ID), is that what we want? I've mentioned my concern around the "op"
values in these records earlier in the patchset, that still applies
here, but now I'm also concerned about the multiple records. I'm
thinking we might be better served with a single record with either
multiple "contid" fields, or a single "contid" field with a set of
comma separated values (or some other delimiter that Steve's tools
will tolerate).
Steve, thoughts?
> + }
> +}
> +EXPORT_SYMBOL(audit_log_contid_list);
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -2132,17 +2146,21 @@ int audit_log_contid(struct audit_context *context,
> char *op, u64 contid)
> {
> struct audit_buffer *ab;
> + gfp_t gfpflags;
>
> if (!cid_valid(contid))
> return 0;
> + /* We can be called in atomic context via audit_tg() */
> + gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
See my previous comments in the earlier patches about guessing at
gfpflags; let's just add a gfpflags parameter to audit_log_contid().
> /* Generate AUDIT_CONTAINER record with container ID */
> - ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> + ab = audit_log_start(context, gfpflags, AUDIT_CONTAINER);
> if (!ab)
> return -ENOMEM;
> audit_log_format(ab, "op=%s contid=%llu", op, contid);
> audit_log_end(ab);
> return 0;
> }
> +EXPORT_SYMBOL(audit_log_contid);
Move the EXPORT_SYMBOL() to earlier in the patchset when you first
define audit_log_contid().
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6ab5e5e..e2a16d2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1015,6 +1015,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
> }
> +EXPORT_SYMBOL(audit_alloc_local);
Same as above.
> void audit_free_context(struct audit_context *context)
> {
> @@ -1029,6 +1030,7 @@ void audit_free_context(struct audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
Same.
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index f368ee6..10d2707 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == 0)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local();
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,12 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_contid_list(net, context);
> +
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 09/10] debug audit: read container ID of a process
From: Paul Moore @ 2018-07-20 22:15 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <b1eff0b849cc472f10dee8bd4f2eeb3567f0a459.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:02 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add support for reading the audit container identifier from the proc
> filesystem.
>
> This is a read from the proc entry of the form
> /proc/PID/audit_containerid where PID is the process ID of the task
> whose audit container identifier is sought.
>
> The read expects up to a u64 value (unset: 18446744073709551615).
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/proc/base.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 318dff4..ca8bfe2 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1303,6 +1303,21 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> .llseek = generic_file_llseek,
> };
>
> +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + struct task_struct *task = get_proc_task(inode);
> + ssize_t length;
> + char tmpbuf[TMPBUFLEN*2];
> +
> + if (!task)
> + return -ESRCH;
> + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> + put_task_struct(task);
> + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
While I still remain very nervous about opening the audit container ID
up for abuse by making it accessible, I understand that this would
make things a lot easier us (e.g. testing) and perhaps the container
engines as well. In order to limit the potential for abuse, what do
you think about restricting read access to those processes which have
CAP_AUDIT_CONTROL, similar to what we do for setting the audit
container ID?
> static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -1333,6 +1348,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> }
>
> static const struct file_operations proc_contid_operations = {
> + .read = proc_contid_read,
> .write = proc_contid_write,
> .llseek = generic_file_llseek,
> };
> @@ -3030,7 +3046,7 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
> #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3422,7 +3438,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 08/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Laura Garcia @ 2018-07-21 15:32 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, luto, jlayton, carlos, viro,
dhowells, simo, eparis, serge, ebiederm,
Netfilter Development Mailing list
In-Reply-To: <c6b64f6540e2e531636142ced36d51f0744d0e1f.1528304204.git.rgb@redhat.com>
CC'ing Netfilter.
On Wed, Jun 6, 2018 at 6:58 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 5 +++++
> kernel/audit.c | 20 +++++++++++++++++++-
> kernel/auditsc.c | 2 ++
> net/netfilter/xt_AUDIT.c | 12 ++++++++++--
> 4 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 7e2e51c..4560a4e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -167,6 +167,8 @@ extern int audit_log_contid(struct audit_context *context,
> extern void audit_contid_add(struct net *net, u64 contid);
> extern void audit_contid_del(struct net *net, u64 contid);
> extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
> +extern void audit_log_contid_list(struct net *net,
> + struct audit_context *context);
>
> extern int audit_update_lsm_rules(void);
>
> @@ -231,6 +233,9 @@ static inline void audit_contid_del(struct net *net, u64 contid)
> { }
> static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> { }
> +static inline void audit_log_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled 0
> #endif /* CONFIG_AUDIT */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ecd2de4..8cca41a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -382,6 +382,20 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_contid_add(new->net_ns, contid);
> }
>
> +void audit_log_contid_list(struct net *net, struct audit_context *context)
> +{
> + struct audit_contid *cont;
> + int i = 0;
> +
> + list_for_each_entry(cont, audit_get_contid_list(net), list) {
> + char buf[14];
> +
> + sprintf(buf, "net%u", i++);
> + audit_log_contid(context, buf, cont->id);
> + }
> +}
> +EXPORT_SYMBOL(audit_log_contid_list);
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -2132,17 +2146,21 @@ int audit_log_contid(struct audit_context *context,
> char *op, u64 contid)
> {
> struct audit_buffer *ab;
> + gfp_t gfpflags;
>
> if (!cid_valid(contid))
> return 0;
> + /* We can be called in atomic context via audit_tg() */
> + gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
> /* Generate AUDIT_CONTAINER record with container ID */
> - ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> + ab = audit_log_start(context, gfpflags, AUDIT_CONTAINER);
> if (!ab)
> return -ENOMEM;
> audit_log_format(ab, "op=%s contid=%llu", op, contid);
> audit_log_end(ab);
> return 0;
> }
> +EXPORT_SYMBOL(audit_log_contid);
>
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6ab5e5e..e2a16d2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1015,6 +1015,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
> }
> +EXPORT_SYMBOL(audit_alloc_local);
>
> void audit_free_context(struct audit_context *context)
> {
> @@ -1029,6 +1030,7 @@ void audit_free_context(struct audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index f368ee6..10d2707 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == 0)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local();
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,12 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_contid_list(net, context);
> +
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox