* Re: [PATCH net-next 0/4] mm,tcp: provide mmap_hook to solve lockdep issue
From: Eric Dumazet @ 2018-04-23 21:38 UTC (permalink / raw)
To: Andy Lutomirski, Eric Dumazet, David S . Miller
Cc: netdev, linux-kernel, Soheil Hassas Yeganeh, Eric Dumazet,
linux-mm, Linux API
In-Reply-To: <9ed6083f-d731-945c-dbcd-f977c5600b03@kernel.org>
Hi Andy
On 04/23/2018 02:14 PM, Andy Lutomirski wrote:
> On 04/20/2018 08:55 AM, Eric Dumazet wrote:
>> This patch series provide a new mmap_hook to fs willing to grab
>> a mutex before mm->mmap_sem is taken, to ensure lockdep sanity.
>>
>> This hook allows us to shorten tcp_mmap() execution time (while mmap_sem
>> is held), and improve multi-threading scalability.
>>
>
> I think that the right solution is to rework mmap() on TCP sockets a bit. The current approach in net-next is very strange for a few reasons:
>
> 1. It uses mmap() as an operation that has side effects besides just creating a mapping. If nothing else, it's surprising, since mmap() doesn't usually do that. But it's also causing problems like what you're seeing.
>
> 2. The performance is worse than it needs to be. mmap() is slow, and I doubt you'll find many mm developers who consider this particular abuse of mmap() to be a valid thing to optimize for.
>
> 3. I'm not at all convinced the accounting is sane. As far as I can tell, you're allowing unprivileged users to increment the count on network-owned pages, limited only by available virtual memory, without obviously charging it to the socket buffer limits. It looks like a program that simply forgot to call munmap() would cause the system to run out of memory, and I see no reason to expect the OOM killer to have any real chance of killing the right task.
>
> 4. Error handling sucks. If I try to mmap() a large range (which is the whole point -- using a small range will kill performance) and not quite all of it can be mapped, then I waste a bunch of time in the kernel and get *none* of the range mapped.
>
> I would suggest that you rework the interface a bit. First a user would call mmap() on a TCP socket, which would create an empty VMA. (It would set vm_ops to point to tcp_vm_ops or similar so that the TCP code could recognize it, but it would have no effect whatsoever on the TCP state machine. Reading the VMA would get SIGBUS.) Then a user would call a new ioctl() or setsockopt() function and pass something like:
>
> struct tcp_zerocopy_receive {
> void *address;
> size_t length;
> };
>
> The kernel would verify that [address, address+length) is entirely inside a single TCP VMA and then would do the vm_insert_range magic.
I have no idea what is the proper API for that.
Where the TCP VMA(s) would be stored ?
In TCP socket, or MM layer ?
And I am not sure why the error handling would be better (point 4), unless we can return smaller @length than requested maybe ?
Also how the VMA space would be accounted (point 3) when creating an empty VMA (no pages in there yet)
On success, length is changed to the length that actually got mapped. The kernel could do this while holding mmap_sem for *read*, and it could get the lock ordering right. If and when mm range locks ever get merged, it could switch to using a range lock.
>
> Then you could use MADV_DONTNEED or another ioctl/setsockopt to zap the part of the mapping that you're done with.
>
> Does this seem reasonable? It should involve very little code change, it will run faster, it will scale better, and it is much less weird IMO.
Maybe, although I do not see the 'little code change' yet.
But at least, this seems pretty nice idea, especially if it could allow us to fill the mmap()ed area later when packets are received.
^ permalink raw reply
* Re: [PATCH net-next 0/4] mm,tcp: provide mmap_hook to solve lockdep issue
From: Andy Lutomirski @ 2018-04-23 21:14 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller
Cc: netdev, linux-kernel, Soheil Hassas Yeganeh, Eric Dumazet,
linux-mm, Linux API
In-Reply-To: <20180420155542.122183-1-edumazet@google.com>
On 04/20/2018 08:55 AM, Eric Dumazet wrote:
> This patch series provide a new mmap_hook to fs willing to grab
> a mutex before mm->mmap_sem is taken, to ensure lockdep sanity.
>
> This hook allows us to shorten tcp_mmap() execution time (while mmap_sem
> is held), and improve multi-threading scalability.
>
I think that the right solution is to rework mmap() on TCP sockets a
bit. The current approach in net-next is very strange for a few reasons:
1. It uses mmap() as an operation that has side effects besides just
creating a mapping. If nothing else, it's surprising, since mmap()
doesn't usually do that. But it's also causing problems like what
you're seeing.
2. The performance is worse than it needs to be. mmap() is slow, and I
doubt you'll find many mm developers who consider this particular abuse
of mmap() to be a valid thing to optimize for.
3. I'm not at all convinced the accounting is sane. As far as I can
tell, you're allowing unprivileged users to increment the count on
network-owned pages, limited only by available virtual memory, without
obviously charging it to the socket buffer limits. It looks like a
program that simply forgot to call munmap() would cause the system to
run out of memory, and I see no reason to expect the OOM killer to have
any real chance of killing the right task.
4. Error handling sucks. If I try to mmap() a large range (which is the
whole point -- using a small range will kill performance) and not quite
all of it can be mapped, then I waste a bunch of time in the kernel and
get *none* of the range mapped.
I would suggest that you rework the interface a bit. First a user would
call mmap() on a TCP socket, which would create an empty VMA. (It would
set vm_ops to point to tcp_vm_ops or similar so that the TCP code could
recognize it, but it would have no effect whatsoever on the TCP state
machine. Reading the VMA would get SIGBUS.) Then a user would call a
new ioctl() or setsockopt() function and pass something like:
struct tcp_zerocopy_receive {
void *address;
size_t length;
};
The kernel would verify that [address, address+length) is entirely
inside a single TCP VMA and then would do the vm_insert_range magic. On
success, length is changed to the length that actually got mapped. The
kernel could do this while holding mmap_sem for *read*, and it could get
the lock ordering right. If and when mm range locks ever get merged, it
could switch to using a range lock.
Then you could use MADV_DONTNEED or another ioctl/setsockopt to zap the
part of the mapping that you're done with.
Does this seem reasonable? It should involve very little code change,
it will run faster, it will scale better, and it is much less weird IMO.
^ permalink raw reply
* Re: [PATCH RFC v5] pidns: introduce syscall translate_pid
From: Nagarathnam Muthusamy @ 2018-04-23 17:37 UTC (permalink / raw)
To: Konstantin Khlebnikov, Eric W. Biederman
Cc: linux-api, linux-kernel, Jann Horn, Serge Hallyn, Oleg Nesterov,
Andy Lutomirski, Prakash Sangappa, Andrew Morton
In-Reply-To: <112c7cac-1982-3a2e-ffc0-878bc5ae4bb6@yandex-team.ru>
On 04/05/2018 12:02 AM, Konstantin Khlebnikov wrote:
> On 05.04.2018 01:29, Eric W. Biederman wrote:
>> Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com> writes:
>>
>>> On 04/04/2018 12:11 PM, Konstantin Khlebnikov wrote:
>>>> Each process have different pids, one for each pid namespace it
>>>> belongs.
>>>> When interaction happens within single pid-ns translation isn't
>>>> required.
>>>> More complicated scenarios needs special handling.
>>>>
>>>> For example:
>>>> - reading pid-files or logs written inside container with pid
>>>> namespace
>>>> - attaching with ptrace to tasks from different pid namespace
>>>> - passing pids across pid namespaces in any kind of API
>>>>
>>>> Currently there are several interfaces that could be used here:
>>>>
>>>> Pid namespaces are identified by inode number of /proc/[pid]/ns/pid.
>>
>> Using the inode number in interfaces is not an option. Especially not
>> withou referencing the device number for the filesystem as well.
>
> This is supposed to be single-instance fs,
> not part of proc but referenced but its magic "symlinks".
>
> Device numbers are not mentioned in "man namespaces".
>
>>
>>>> Pids for nested Pid namespaces are shown in file /proc/[pid]/status.
>>>> In some cases conversion pid -> vpid could be easily done using this
>>>> information, but backward translation requires scanning all tasks.
>>>>
>>>> Unix socket automatically translates pid attached to SCM_CREDENTIALS.
>>>> This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
>>>> into pid namespace, this expose process and could be insecure.
>>>>
>>>> This patch adds new syscall for converting pids between pid
>>>> namespaces:
>>>>
>>>> pid_t translate_pid(pid_t pid, int source_type, int source,
>>>> int target_type, int target);
>>>>
>>>> @source_type and @target_type defines type of following arguments:
>>>>
>>>> TRANSLATE_PID_CURRENT_PIDNS - current pid namespace, argument is
>>>> unused
>>>> TRANSLATE_PID_TASK_PIDNS - task pid-ns, argument is task pid
>>>
>>> I believe using pid to represent the namespace has been already
>>> discussed in V1 of this patch in https://lkml.org/lkml/2015/9/22/1087
>>> after which we moved on to fd based version of this interface.
>>
>> Or in short why is the case of pids important?
>>
>> You Konstantin you almost said why they were important in your message
>> saying you were going to send this one. However you don't explain in
>> your description why you want to identify pid namespaces by pid.
>>
>
> Open of /proc/[pid]/ns/pid requires same permissions as ptrace,
> pid based variant doesn't have such restrictions.
Can you provide more information on usecase requiring PID translation
but not used for tracing related purposes?
On a side note, can we have the types TRANSLATE_PID_CURRENT_PIDNS and
TRANSLATE_PID_FD_PIDNS integrated first and then possibly extend the
interface to include TRANSLATE_PID_TASK_PIDNS in future?
Thanks,
Nagarathnam.
> Most pid-based syscalls are racy in some cases but they are
> here for decades and everybody knowns how to deal with it.
> So, I've decided to merge both worlds in one interface which clearly
> tells what to expect.
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Mike Kravetz @ 2018-04-23 4:22 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, linux-kernel, linux-api, Reinette Chatre,
Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, Vlastimil Babka, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton
In-Reply-To: <20180423000943.GO17484@dhcp22.suse.cz>
On 04/22/2018 05:09 PM, Michal Hocko wrote:
> On Mon 16-04-18 19:09:14, Mike Kravetz wrote:
> [...]
>> @@ -2010,9 +2011,13 @@ static __always_inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>> {
>> return __rmqueue_smallest(zone, order, MIGRATE_CMA);
>> }
>> +#define contig_alloc_migratetype_ok(migratetype) \
>> + ((migratetype) == MIGRATE_CMA || (migratetype) == MIGRATE_MOVABLE)
>> #else
>> static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>> unsigned int order) { return NULL; }
>> +#define contig_alloc_migratetype_ok(migratetype) \
>> + ((migratetype) == MIGRATE_MOVABLE)
>> #endif
>>
>> /*
>> @@ -7822,6 +7827,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>> };
>> INIT_LIST_HEAD(&cc.migratepages);
>>
>> + if (!contig_alloc_migratetype_ok(migratetype))
>> + return -EINVAL;
>> +
>>
>> /*
>> * What we do here is we mark all pageblocks in range as
>> * MIGRATE_ISOLATE. Because pageblock and max order pages may
>> @@ -7912,8 +7920,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>>
>> /* Make sure the range is really isolated. */
>> if (test_pages_isolated(outer_start, end, false)) {
>> - pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
>> - __func__, outer_start, end);
>> + if (!(migratetype == MIGRATE_MOVABLE)) /* only print for CMA */
>> + pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
>> + __func__, outer_start, end);
>> ret = -EBUSY;
>> goto done;
>> }
>
> This probably belongs to a separate patch. I would be tempted to say
> that we should get rid of this migratetype thingy altogether. I confess
> I have forgot everything about why this is required actually but it is
> ugly as hell. Not your fault of course.
>
>> @@ -7949,6 +7958,82 @@ void free_contig_range(unsigned long pfn, unsigned long nr_pages)
>> }
>> WARN(count != 0, "%ld pages are still in use!\n", count);
>> }
>> +
>> +static bool contig_pfn_range_valid(struct zone *z, unsigned long start_pfn,
>> + unsigned long nr_pages)
>> +{
>> + unsigned long i, end_pfn = start_pfn + nr_pages;
>> + struct page *page;
>> +
>> + for (i = start_pfn; i < end_pfn; i++) {
>> + if (!pfn_valid(i))
>> + return false;
>> +
>> + page = pfn_to_page(i);
>
> It believe we want pfn_to_online_page here. The old giga pages code is
> buggy in that regard but nothing really critical because the
> alloc_contig_range will notice that.
Ok
> Also do we want to check other usual suspects? E.g. PageReserved? And
> generally migrateable pages if page count > 0. Or do we want to leave
> everything to the alloc_contig_range?
I think you proposed something like the above with limited checking at
some time in the past. In my testing, allocations were more likely to
succeed if we did limited testing here and let alloc_contig_range take
a shot at migration/allocation. There really are two ways to approach
this, do as much checking up front or let it be handled by alloc_contig_range.
>> +
>> + if (page_zone(page) != z)
>> + return false;
>> +
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/**
>> + * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
>> + * range of pages
>> + * @order: number of pages
>> + * @gfp: gfp mask used to limit search as well as during compaction
>> + * @nid: target node
>> + * @nodemask: mask of other possible nodes
>> + *
>> + * Pages can be freed with a call to free_contig_pages(), or by manually
>> + * calling __free_page() for each page allocated.
>> + *
>> + * Return: pointer to 'order' pages on success, or NULL if not successful.
>> + */
>> +struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
>> + int nid, nodemask_t *nodemask)
>
> Vlastimil asked about this but I would even say that we do not want to
> make this order based. Why would we want to restrict the api to 2^order
> sizes in the first place? What if somebody wants to allocate 123 pages?
After Vlastimil's question and again here, I realized that this routine
has a HUGE gap. For allocation sizes less than MAX_ORDER, it should be
using the traditional paga allocation routines. It is only when allocation
size is greater than MAX_ORDER that we need to call into alloc_contig_range.
Unless I am missing something, calls to alloc_contig range need to have
a size that is a multiple of page block. This is because isolation needs
to take place at a page block level. We can easily 'round up' and release
excess pages.
Using number of pages instead of order makes sense. I will rewrite with
this in mind as well as taking the other issues into account.
>
>> +{
>> + unsigned long pfn, nr_pages, flags;
>> + struct page *ret_page = NULL;
>> + struct zonelist *zonelist;
>> + struct zoneref *z;
>> + struct zone *zone;
>> + int rc;
>> +
>> + nr_pages = 1 << order;
>> + zonelist = node_zonelist(nid, gfp);
>> + for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp),
>> + nodemask) {
>> + spin_lock_irqsave(&zone->lock, flags);
>> + pfn = ALIGN(zone->zone_start_pfn, nr_pages);
>> + while (zone_spans_pfn(zone, pfn + nr_pages - 1)) {
>> + if (contig_pfn_range_valid(zone, pfn, nr_pages)) {
>> + spin_unlock_irqrestore(&zone->lock, flags);
>
> I know that the giga page allocation does use the zone lock but why? I
> suspect it wants to stabilize zone_start_pfn but zone lock doesn't do
> that.
>
I am not sure. As you suspected, this was copied from the giga page
allocation code. I'll look into it.
>> +
>> + rc = alloc_contig_range(pfn, pfn + nr_pages,
>> + MIGRATE_MOVABLE, gfp);
>> + if (!rc) {
>> + ret_page = pfn_to_page(pfn);
>> + return ret_page;
>> + }
>> + spin_lock_irqsave(&zone->lock, flags);
>> + }
>> + pfn += nr_pages;
>> + }
>> + spin_unlock_irqrestore(&zone->lock, flags);
>> + }
>
> Other than that this API looks much saner than alloc_contig_range. We
> still need to sort out some details (e.g. alignment) but it should be an
> improvement.
Ok, I will continue to refine. We now have at least one immediate use
case for this type of functionality.
--
Mike Kravetz
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Michal Hocko @ 2018-04-23 0:09 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-mm, linux-kernel, linux-api, Reinette Chatre,
Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, Vlastimil Babka, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton
In-Reply-To: <20180417020915.11786-3-mike.kravetz@oracle.com>
On Mon 16-04-18 19:09:14, Mike Kravetz wrote:
[...]
> @@ -2010,9 +2011,13 @@ static __always_inline struct page *__rmqueue_cma_fallback(struct zone *zone,
> {
> return __rmqueue_smallest(zone, order, MIGRATE_CMA);
> }
> +#define contig_alloc_migratetype_ok(migratetype) \
> + ((migratetype) == MIGRATE_CMA || (migratetype) == MIGRATE_MOVABLE)
> #else
> static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
> unsigned int order) { return NULL; }
> +#define contig_alloc_migratetype_ok(migratetype) \
> + ((migratetype) == MIGRATE_MOVABLE)
> #endif
>
> /*
> @@ -7822,6 +7827,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> };
> INIT_LIST_HEAD(&cc.migratepages);
>
> + if (!contig_alloc_migratetype_ok(migratetype))
> + return -EINVAL;
> +
>
> /*
> * What we do here is we mark all pageblocks in range as
> * MIGRATE_ISOLATE. Because pageblock and max order pages may
> @@ -7912,8 +7920,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>
> /* Make sure the range is really isolated. */
> if (test_pages_isolated(outer_start, end, false)) {
> - pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
> - __func__, outer_start, end);
> + if (!(migratetype == MIGRATE_MOVABLE)) /* only print for CMA */
> + pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
> + __func__, outer_start, end);
> ret = -EBUSY;
> goto done;
> }
This probably belongs to a separate patch. I would be tempted to say
that we should get rid of this migratetype thingy altogether. I confess
I have forgot everything about why this is required actually but it is
ugly as hell. Not your fault of course.
> @@ -7949,6 +7958,82 @@ void free_contig_range(unsigned long pfn, unsigned long nr_pages)
> }
> WARN(count != 0, "%ld pages are still in use!\n", count);
> }
> +
> +static bool contig_pfn_range_valid(struct zone *z, unsigned long start_pfn,
> + unsigned long nr_pages)
> +{
> + unsigned long i, end_pfn = start_pfn + nr_pages;
> + struct page *page;
> +
> + for (i = start_pfn; i < end_pfn; i++) {
> + if (!pfn_valid(i))
> + return false;
> +
> + page = pfn_to_page(i);
It believe we want pfn_to_online_page here. The old giga pages code is
buggy in that regard but nothing really critical because the
alloc_contig_range will notice that.
Also do we want to check other usual suspects? E.g. PageReserved? And
generally migrateable pages if page count > 0. Or do we want to leave
everything to the alloc_contig_range?
> +
> + if (page_zone(page) != z)
> + return false;
> +
> + }
> +
> + return true;
> +}
> +
> +/**
> + * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
> + * range of pages
> + * @order: number of pages
> + * @gfp: gfp mask used to limit search as well as during compaction
> + * @nid: target node
> + * @nodemask: mask of other possible nodes
> + *
> + * Pages can be freed with a call to free_contig_pages(), or by manually
> + * calling __free_page() for each page allocated.
> + *
> + * Return: pointer to 'order' pages on success, or NULL if not successful.
> + */
> +struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
> + int nid, nodemask_t *nodemask)
Vlastimil asked about this but I would even say that we do not want to
make this order based. Why would we want to restrict the api to 2^order
sizes in the first place? What if somebody wants to allocate 123 pages?
> +{
> + unsigned long pfn, nr_pages, flags;
> + struct page *ret_page = NULL;
> + struct zonelist *zonelist;
> + struct zoneref *z;
> + struct zone *zone;
> + int rc;
> +
> + nr_pages = 1 << order;
> + zonelist = node_zonelist(nid, gfp);
> + for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp),
> + nodemask) {
> + spin_lock_irqsave(&zone->lock, flags);
> + pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> + while (zone_spans_pfn(zone, pfn + nr_pages - 1)) {
> + if (contig_pfn_range_valid(zone, pfn, nr_pages)) {
> + spin_unlock_irqrestore(&zone->lock, flags);
I know that the giga page allocation does use the zone lock but why? I
suspect it wants to stabilize zone_start_pfn but zone lock doesn't do
that.
> +
> + rc = alloc_contig_range(pfn, pfn + nr_pages,
> + MIGRATE_MOVABLE, gfp);
> + if (!rc) {
> + ret_page = pfn_to_page(pfn);
> + return ret_page;
> + }
> + spin_lock_irqsave(&zone->lock, flags);
> + }
> + pfn += nr_pages;
> + }
> + spin_unlock_irqrestore(&zone->lock, flags);
> + }
Other than that this API looks much saner than alloc_contig_range. We
still need to sort out some details (e.g. alignment) but it should be an
improvement.
Thanks!
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] x86: ipc: fix x32 version of shmid64_ds and msqid64_ds
From: Arnd Bergmann @ 2018-04-22 20:17 UTC (permalink / raw)
To: H.J. Lu
Cc: Jeffrey Walton, the arch/x86 maintainers, Thomas Gleixner,
Ingo Molnar, Eric W . Biederman, y2038 Mailman List, LKML,
Linux API, linux-arch, GNU C Library, Deepa Dinamani, Al Viro,
Albert ARIBAUD, linux-s390, Martin Schwidefsky, Catalin Marinas,
Will Deacon, open list:RALINK MIPS ARCHITECTURE, James Hogan,
Ralf Baechle, linuxppc-dev, sparclin
In-Reply-To: <CAMe9rOrTzgDmCF+A7nYdGR6BHZBuK5SMnJxeRxBFQW00VmdXvg@mail.gmail.com>
On Sun, Apr 22, 2018 at 2:38 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Apr 20, 2018 at 7:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Apr 20, 2018 at 3:53 PM, Jeffrey Walton <noloader@gmail.com> wrote:
>
> Glibc has correct header files for system calls. I have a very old
> program to check if Linux kernel header files are correct for user
> space:
>
> https://github.com/hjl-tools/linux-header
>
> It needs update to check uapi.
Simply running 'make' on a regular distro shows this output:
--- kernel.x32.out 2018-04-22 22:10:16.053432423 +0200
+++ glibc.x32.out 2018-04-22 22:10:16.073432838 +0200
@@ -10,9 +10,9 @@ size of daddr_t: 4
size of __ipc_pid_t: 4
size of struct ipc_perm: 48
size of mqd_t: 4
-size of struct msqid_ds: 144
+size of struct msqid_ds: 120
size of struct semid_ds: 104
-size of struct shmid_ds: 136
+size of struct shmid_ds: 112
size of struct shminfo: 72
size of struct timeval: 16
size of struct timespec: 16
@@ -22,8 +22,8 @@ size of struct mq_attr: 64
size of struct rlimit: 16
size of struct rusage: 144
size of struct stat: 144
-size of struct statfs: 64
-size of struct statfs64: 88
+size of struct statfs: 120
+size of struct statfs64: 120
size of struct timex: 208
size of struct msginfo: 32
size of struct msgbuf: 16
This seems plausible, the statfs structure clearly has the same problem
as msqid_ds/shmid_ds based on its usage of '__statfs_word' which is
now defined as '__u32' rather than '__kernel_long_t'.
It should be trivial to override __statfs_word from
arch/x86/include/uapi/asm/statfs.h
I've checked the other uses of __BITS_PER_LONG in the uapi
headers now, and all the others are either not relevant for x32
(either definition is fine) or it has to be __BITS_PER_LONG=32.
Arnd
^ permalink raw reply
* Re: sendmmsg flags userspace ABI change in kernel 4.6
From: Mark Wielaard @ 2018-04-22 16:10 UTC (permalink / raw)
To: Florian Weimer, linux-api, netdev, linux-sctp
Cc: linux-kernel, Tom Herbert, valgrind-developers
In-Reply-To: <874lk8jq04.fsf@mid.deneb.enyo.de>
Hi,
Adding valgrind-developers to CC.
On Wed, 2018-04-18 at 21:03 +0200, Florian Weimer wrote:
> Since this commit:
>
> commit 28a94d8fb35b3a75b802f368ae6f4a9f6b0d435a
> Author: Tom Herbert <tom@herbertland.com>
> Date: Mon Mar 7 14:11:02 2016 -0800
>
> net: Allow MSG_EOR in each msghdr of sendmmsg
>
> This patch allows setting MSG_EOR in each individual msghdr passed
> in sendmmsg. This allows a sendmmsg to send multiple messages when
> using SOCK_SEQPACKET.
>
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> the msg_flags argument in individual msghdr arguments is longer
> completely ignored for SOCK_SEQPACKET sockets. msg_flags was and is
> still documented as ignored for sendmsg(2), so by analogy for
> sendmmsg(2) as well.
>
> It seems that valgrind does not know about this yet, and due to
> limited use of SCTP, this userspace ABI change has not been noticed so
> far.
That is correct. If you look at coregrind/m_syswrap/syswrap-generic.c
(msghdr_foreachfield), it explicitly says:
/* msg_flags is completely ignored for send_mesg, recv_mesg doesn't read
the field, but does write to it. */
So the various valgrind syscall wrappers will not be called to inspect
the msg_flags field. This means you don't get warned if the msg_flags
field contains undefined bits for any syscall send_msg variant.
> What are the plans in this area? Will other kinds of sockets start
> using the msghdr flags for sending?
>
> A fully backwards-compatibility way to achieve this would be to
> specify that you have to pass a new flag to sendmmsg (MSG_PERHDR?), in
> its flags argument, to activate the per-msghdr flags.
>
> The glibc DNS stub resolver relies on the previously documented
> behavior, and I wonder how widely we should backport the change:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=23037
>
> If the MSG_PERHDR route will be taken, we can skip this work, and
> valgrind can flag uninitialized bits in msg_flags only if MSG_PERHDR
> is passed. (I believe it would be difficult for valgrind to look at
> the socket type to determine whether undefined bits need reporting.)
We have abstracted the checking a bit so it can be reused for the
different ways a msg can be send, through socketcall, sendmsg and
sendmmsg. I am not sure we can easily determine the underlying socket
type. It seems to mean we would have to track the creation of all
socketfds. So having a flag bit for sendmsg and sendmmsg would be much
nicer.
Thanks,
Mark
^ permalink raw reply
* Re: [PATCH 07/24] hibernate: Disable when the kernel is locked down
From: Andy Lutomirski @ 2018-04-22 14:34 UTC (permalink / raw)
To: David Howells
Cc: Pavel Machek, Linus Torvalds, linux-man, Linux API, James Morris,
LKML, LSM List
In-Reply-To: <27926.1524148733@warthog.procyon.org.uk>
On Thu, Apr 19, 2018 at 7:38 AM, David Howells <dhowells@redhat.com> wrote:
> Pavel Machek <pavel@ucw.cz> wrote:
>
>> > There is currently no way to verify the resume image when returning
>> > from hibernate. This might compromise the signed modules trust model,
>> > so until we can work with signed hibernate images we disable it when the
>> > kernel is locked down.
>>
>> I'd rather see hibernation fixed than disabled like this.
>
> The problem is that you have to store the hibernated kernel image encrypted,
> but you can't store the decryption key on disk unencrypted or you've just
> wasted the effort.
>
> So the firmware has to unlock the image, asking the user for a password to
> unlock the key.
Why firmware?
Either the boot kernel could figure out how to ask for a password (or
unseal using the TPM) or we could defer this to userspace. The latter
should already work using kexec-jump, no?
^ permalink raw reply
* Re: [PATCH] x86: ipc: fix x32 version of shmid64_ds and msqid64_ds
From: H.J. Lu @ 2018-04-22 12:38 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:RALINK MIPS ARCHITECTURE, James Hogan, Will Deacon,
Albert ARIBAUD, Deepa Dinamani, H. Peter Anvin, sparclinux,
linux-arch, linux-s390, y2038 Mailman List,
the arch/x86 maintainers, Ingo Molnar, Thorsten Glaser,
Catalin Marinas, Ben Hutchings, Adam Borowski, Daniel Schepler,
Al Viro, John Paul Adrian Glaubitz, Thomas Gleixner,
Jeffrey Walton, GNU C Library
In-Reply-To: <CAK8P3a0nL4B+t6BMRhr36RrZ_jDgwnY1BviNPD+cFzsUGeppmA@mail.gmail.com>
On Fri, Apr 20, 2018 at 7:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Apr 20, 2018 at 3:53 PM, Jeffrey Walton <noloader@gmail.com> wrote:
>>> +#if !defined(__x86_64__) || !defined(__ilp32__)
>>> #include <asm-generic/msgbuf.h>
>>> +#else
>>
>> I understand there's some progress having Clang compile the kernel.
>> Clang treats __ILP32__ and friends differently than GCC. I believe
>> ILP32 shows up just about everywhere there are 32-bit ints, longs and
>> pointers. You might find it on Aarch64 or you might find it on MIPS64
>> when using Clang.
>>
>> I think that means this may be a little suspicious:
>>
>> > +#if !defined(__x86_64__) || !defined(__ilp32__)
>>
>> I kind of felt LLVM was wandering away from the x32 ABI, but the LLVM
>> devs insisted they were within their purview. Also see
>> https://lists.llvm.org/pipermail/cfe-dev/2015-December/046300.html.
>>
>> Sorry about the top-post. I just wanted to pick out that one piece.
>
> It seems I made a typo and it needs to be __ILP32__ rather than
> __ilp32__ (corrected that locally, will resend once we have resolved
> this).
>
> Aside from that, the #if check seems to be correct to me: this
> is an x86-specific header, so it won't ever be seen on other
> architectures. On x86-32, __x86_64__ isn't set, so we don't care
> about whether __ilp32__ is set or not, and on x86-64 (lp64),
> __ilp32__ is never set, so we still get the asm-generic header.
>
Glibc has correct header files for system calls. I have a very old
program to check if Linux kernel header files are correct for user
space:
https://github.com/hjl-tools/linux-header
It needs update to check uapi.
--
H.J.
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Vlastimil Babka @ 2018-04-21 16:16 UTC (permalink / raw)
To: Mike Kravetz, linux-mm, linux-kernel, linux-api
Cc: Reinette Chatre, Michal Hocko, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton
In-Reply-To: <20180417020915.11786-3-mike.kravetz@oracle.com>
On 04/17/2018 04:09 AM, Mike Kravetz wrote:
> find_alloc_contig_pages() is a new interface that attempts to locate
> and allocate a contiguous range of pages. It is provided as a more
> convenient interface than alloc_contig_range() which is currently
> used by CMA and gigantic huge pages.
>
> When attempting to allocate a range of pages, migration is employed
> if possible. There is no guarantee that the routine will succeed.
> So, the user must be prepared for failure and have a fall back plan.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Hi, just two quick observations, maybe discussion pointers for the
LSF/MM session:
- it's weird that find_alloc_contig_pages() takes an order, and
free_contig_pages() takes a nr_pages. I suspect the interface would be
more future-proof with both using nr_pages? Perhaps also minimum
alignment for the allocation side? Order is fine for hugetlb, but what
about other potential users?
- contig_alloc_migratetype_ok() says that MIGRATE_CMA blocks are OK to
allocate from. This silently assumes that everything allocated by this
will be migratable itself, or it might eat CMA reserves. Is it the case?
Also you then call alloc_contig_range() with MIGRATE_MOVABLE, so it will
skip/fail on MIGRATE_CMA anyway IIRC.
Vlastimil
> ---
> include/linux/gfp.h | 12 ++++++++
> mm/page_alloc.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 99 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 86a0d06463ab..528b194cc266 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -573,6 +573,18 @@ static inline bool pm_suspended_storage(void)
> extern int alloc_contig_range(unsigned long start, unsigned long end,
> unsigned migratetype, gfp_t gfp_mask);
> extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
> +extern struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
> + int nid, nodemask_t *nodemask);
> +extern void free_contig_pages(struct page *page, unsigned long nr_pages);
> +#else
> +static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
> + int nid, nodemask_t *nodemask)
> +{
> + return NULL;
> +}
> +static void free_contig_pages(struct page *page, unsigned long nr_pages)
> +{
> +}
> #endif
>
> #ifdef CONFIG_CMA
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0fd5e8e2456e..81070fe55c44 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -67,6 +67,7 @@
> #include <linux/ftrace.h>
> #include <linux/lockdep.h>
> #include <linux/nmi.h>
> +#include <linux/mmzone.h>
>
> #include <asm/sections.h>
> #include <asm/tlbflush.h>
> @@ -2010,9 +2011,13 @@ static __always_inline struct page *__rmqueue_cma_fallback(struct zone *zone,
> {
> return __rmqueue_smallest(zone, order, MIGRATE_CMA);
> }
> +#define contig_alloc_migratetype_ok(migratetype) \
> + ((migratetype) == MIGRATE_CMA || (migratetype) == MIGRATE_MOVABLE)
> #else
> static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
> unsigned int order) { return NULL; }
> +#define contig_alloc_migratetype_ok(migratetype) \
> + ((migratetype) == MIGRATE_MOVABLE)
> #endif
>
> /*
> @@ -7822,6 +7827,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> };
> INIT_LIST_HEAD(&cc.migratepages);
>
> + if (!contig_alloc_migratetype_ok(migratetype))
> + return -EINVAL;
> +
> /*
> * What we do here is we mark all pageblocks in range as
> * MIGRATE_ISOLATE. Because pageblock and max order pages may
> @@ -7912,8 +7920,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>
> /* Make sure the range is really isolated. */
> if (test_pages_isolated(outer_start, end, false)) {
> - pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
> - __func__, outer_start, end);
> + if (!(migratetype == MIGRATE_MOVABLE)) /* only print for CMA */
> + pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
> + __func__, outer_start, end);
> ret = -EBUSY;
> goto done;
> }
> @@ -7949,6 +7958,82 @@ void free_contig_range(unsigned long pfn, unsigned long nr_pages)
> }
> WARN(count != 0, "%ld pages are still in use!\n", count);
> }
> +
> +static bool contig_pfn_range_valid(struct zone *z, unsigned long start_pfn,
> + unsigned long nr_pages)
> +{
> + unsigned long i, end_pfn = start_pfn + nr_pages;
> + struct page *page;
> +
> + for (i = start_pfn; i < end_pfn; i++) {
> + if (!pfn_valid(i))
> + return false;
> +
> + page = pfn_to_page(i);
> +
> + if (page_zone(page) != z)
> + return false;
> +
> + }
> +
> + return true;
> +}
> +
> +/**
> + * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
> + * range of pages
> + * @order: number of pages
> + * @gfp: gfp mask used to limit search as well as during compaction
> + * @nid: target node
> + * @nodemask: mask of other possible nodes
> + *
> + * Pages can be freed with a call to free_contig_pages(), or by manually
> + * calling __free_page() for each page allocated.
> + *
> + * Return: pointer to 'order' pages on success, or NULL if not successful.
> + */
> +struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
> + int nid, nodemask_t *nodemask)
> +{
> + unsigned long pfn, nr_pages, flags;
> + struct page *ret_page = NULL;
> + struct zonelist *zonelist;
> + struct zoneref *z;
> + struct zone *zone;
> + int rc;
> +
> + nr_pages = 1 << order;
> + zonelist = node_zonelist(nid, gfp);
> + for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp),
> + nodemask) {
> + spin_lock_irqsave(&zone->lock, flags);
> + pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> + while (zone_spans_pfn(zone, pfn + nr_pages - 1)) {
> + if (contig_pfn_range_valid(zone, pfn, nr_pages)) {
> + spin_unlock_irqrestore(&zone->lock, flags);
> +
> + rc = alloc_contig_range(pfn, pfn + nr_pages,
> + MIGRATE_MOVABLE, gfp);
> + if (!rc) {
> + ret_page = pfn_to_page(pfn);
> + return ret_page;
> + }
> + spin_lock_irqsave(&zone->lock, flags);
> + }
> + pfn += nr_pages;
> + }
> + spin_unlock_irqrestore(&zone->lock, flags);
> + }
> +
> + return ret_page;
> +}
> +EXPORT_SYMBOL_GPL(find_alloc_contig_pages);
> +
> +void free_contig_pages(struct page *page, unsigned long nr_pages)
> +{
> + free_contig_range(page_to_pfn(page), nr_pages);
> +}
> +EXPORT_SYMBOL_GPL(free_contig_pages);
> #endif
>
> #if defined CONFIG_MEMORY_HOTPLUG || defined CONFIG_CMA
>
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Richard Guy Briggs @ 2018-04-21 14:34 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhTyvxxj2e2Gn+iyW6iLLeYB7hp8a+JvfeMmJ2nUPqtEaw@mail.gmail.com>
On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/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).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > 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 container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID. A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > fs/proc/base.c | 37 ++++++++++++++++++++
> > include/linux/audit.h | 16 +++++++++
> > include/linux/init_task.h | 4 ++-
> > include/linux/sched.h | 1 +
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> > .read = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> > };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct inode *inode = file_inode(file);
> > + u64 containerid;
> > + int rv;
> > + struct task_struct *task = get_proc_task(inode);
> > +
> > + if (!task)
> > + return -ESRCH;
> > + if (*ppos != 0) {
> > + /* No partial writes. */
> > + put_task_struct(task);
> > + return -EINVAL;
> > + }
> > +
> > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > + if (rv < 0) {
> > + put_task_struct(task);
> > + return rv;
> > + }
> > +
> > + rv = audit_set_containerid(task, containerid);
> > + put_task_struct(task);
> > + if (rv < 0)
> > + return rv;
> > + return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > + .write = proc_containerid_write,
> > + .llseek = generic_file_llseek,
> > +};
> > +
> > #endif
> >
> > #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,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("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,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("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..fe4ba3f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -29,6 +29,7 @@
> >
> > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > +#define INVALID_CID AUDIT_CID_UNSET
>
> Why can't we just use AUDIT_CID_UNSET? Is there an important
> distinction? If so, they shouldn't they have different values?
One was intended as user-facing and the other was intended for kernel
internal. As you point out, this does not appear to be necessary since
they are both the same type. This was to mirror loginuid due to UID
namespace practice to seperate the two to make things very clear that a
userspace view of a UID needed to be translated from the user's user
namespace to the kernel's absolute view of UIDs from the init user
namespace. Since container ID meanings do not depend on any namespace
context, I agree we can use just one and I'd go with AUDIT_CID_UNSET.
> If we do need to keep INVALID_CID, let's rename it to
> AUDIT_CID_INVALID so we have some consistency to the naming patterns
> and we stress that it is an *audit* container ID.
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d258826..1b82191 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -796,6 +796,7 @@ struct task_struct {
> > #ifdef CONFIG_AUDITSYSCALL
> > kuid_t loginuid;
> > unsigned int sessionid;
> > + u64 containerid;
>
> This one line addition to the task_struct scares me the most of
> anything in this patchset. Why? It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police this.
Fair enough.
> Unfortunately, we can't add the field to audit_context as things
> currently stand because we don't always allocate an audit_context,
> it's dependent on the system's configuration, and we need to track the
> audit container ID for a given process, regardless of the audit
> configuration. Pretty much the same reason why loginuid and sessionid
> are located directly in task_struct now. As I stressed during the
> design phase, I really want to keep this as an *audit* container ID
> and not a general purpose kernel wide container ID. If the kernel
> ever grows a general purpose container ID token, I'll be the first in
> line to convert the audit code, but I don't want audit to be that
> general purpose mechanism ... audit is hated enough as-is ;)
When would we need an audit container ID when audit is not enabled
enough to have an audit_context?
If it is only used for audit, and audit is the only consumer, and audit
can only use it when it is enabled, then we can just return success to
any write to the proc filehandle, or not even present it. Nothing will
be able to know that value wasn't used.
When are loginuid and sessionid used now when audit is not enabled (or
should I say, explicitly disabled)?
> I think the right solution to this is to create another new struct,
> audit_task_info (or similar, the name really isn't that important),
> which would be stored as a pointer in task_struct and would replace
> the audit_context pointer, loginuid, sessionid, and the newly proposed
> containerid. The new audit_task_info would always be allocated in the
> audit_alloc() function (please use kmem_cache), and the audit_context
> pointer included inside would continue to be allocated based on the
> existing conditions. By keeping audit_task_info as a pointer inside
> task_struct we could hide the structure definition inside
> kernel/audit*.c and make it much more difficult for other subsystems
> to abuse it.[1]
>
> struct audit_task_info {
> kuid_t loginuid;
> unsigned int sessionid;
> u64 containerid;
> struct audit_context *ctx;
> }
I agree this looks like a good change.
> Actually, we might even want to consider storing audit_context in
> audit_task_info (no pointer), or making it a zero length array
> (ctx[0]) and going with a variable sized allocation of audit_task_info
> ... but all that could be done as a follow up optimization once we get
> the basic idea sorted.
>
> [1] If for some reason allocating audit_task_info becomes too much
> overhead to bear (somewhat doubtful since we would only do it at task
> creation), we could do some ugly tricks to directly include an
> audit_task_struct chunk in task_struct but I'd like to avoid that if
> possible (and I think we can).
>
> > #endif
> > struct seccomp seccomp;
>
> ...
>
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 4e61a9e..921a71f 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -71,6 +71,7 @@
> > #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */
> > #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or off */
> > #define AUDIT_GET_FEATURE 1019 /* Get which features are enabled */
> > +#define AUDIT_CONTAINER 1020 /* Define the container id and information */
> >
> > #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel */
> > #define AUDIT_USER_AVC 1107 /* We filter this differently */
> > @@ -465,6 +466,7 @@ struct audit_tty_status {
> > };
> >
> > #define AUDIT_UID_UNSET (unsigned int)-1
> > +#define AUDIT_CID_UNSET ((u64)-1)
>
> I think we need to decide if we want to distinguish between the "host"
> (e.g. init ns) and "unset". Looking at this patch (I've only quickly
> skimmed the others so far) it would appear that you don't think we
> need to worry about this distinction; that's fine, but let's make it
> explicit with a comment in the code that AUDIT_CID_UNSET means "unset"
> as well as "host".
I don't see any reason to distinguish between "host" and "unset". Since
a container doesn't have a concrete definition based in namespaces, the
initial namespace set is meaningless here.
Is there value in having a container orchestrator process have a
reserved container ID that has a policy distinct from any other
container? If so, then I could see the value in making the distinction.
For example, I've heard of interest in systemd acting as a container
orchestrator, so if it took on that role as PID 1, then every process in
the system would inherit that ID and none would be unset.
I can't picture how having seperate "host" and "unset" values helps us.
> If we do need to make a distinction, let's add a constant/macro for "host".
Currently "unset" is -1 which fits the convention used for sessionid and
loginuid and a number of others, so I think it makes sense to stick with
that. If we decide we need a "host" flag, would it make sense to use 0
or (u64)-2?
> > /* audit_rule_data supports filter rules with both integer and string
> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 4e0a4ac..29c8482 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> > return rc;
> > }
> >
> > +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> > +{
> > + struct task_struct *parent;
> > + u64 pcontainerid, ccontainerid;
> > +
> > + /* Don't allow to set our own containerid */
> > + if (current == task)
> > + return -EPERM;
>
> Why not? Is there some obvious security concern that I missing?
We then lose the distinction in the AUDIT_CONTAINER record between the
initiating PID and the target PID. This was outlined in the proposal.
Having said that, I'm still not sure we have protected sufficiently from
a child turning around and setting it's parent's as yet unset or
inherited audit container ID.
> I ask because I suppose it might be possible for some container
> runtime to do a fork, setup some of the environment and them exec the
> container (before you answer the obvious "namespaces!" please remember
> we're not trying to define containers).
I don't think namespaces have any bearing on this concern since none are
required.
> > + /* Don't allow the containerid to be unset */
> > + if (!cid_valid(containerid))
> > + return -EINVAL;
> > + /* if we don't have caps, reject */
> > + if (!capable(CAP_AUDIT_CONTROL))
> > + return -EPERM;
> > + /* if containerid is unset, allow */
> > + if (!audit_containerid_set(task))
> > + return 0;
> > + /* it is already set, and not inherited from the parent, reject */
> > + ccontainerid = audit_get_containerid(task);
> > + rcu_read_lock();
> > + parent = rcu_dereference(task->real_parent);
> > + rcu_read_unlock();
> > + task_lock(parent);
> > + pcontainerid = audit_get_containerid(parent);
> > + task_unlock(parent);
> > + if (ccontainerid != pcontainerid)
> > + return -EPERM;
> > + return 0;
> > +}
> > +
> > +static void audit_log_set_containerid(struct task_struct *task, u64 oldcontainerid,
> > + u64 containerid, int rc)
> > +{
> > + struct audit_buffer *ab;
> > + uid_t uid;
> > + struct tty_struct *tty;
> > +
> > + if (!audit_enabled)
> > + return;
> > +
> > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> > + if (!ab)
> > + return;
> > +
> > + uid = from_kuid(&init_user_ns, task_uid(current));
> > + tty = audit_get_tty(current);
> > +
> > + audit_log_format(ab, "op=set pid=%d uid=%u", task_tgid_nr(current), uid);
> > + audit_log_task_context(ab);
> > + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d old-contid=%llu contid=%llu res=%d",
> > + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > + tty ? tty_name(tty) : "(none)", audit_get_sessionid(current),
> > + task_tgid_nr(task), oldcontainerid, containerid, !rc);
> > +
> > + audit_put_tty(tty);
> > + audit_log_end(ab);
> > +}
> > +
> > +/**
> > + * audit_set_containerid - set current task's audit_context containerid
> > + * @containerid: containerid value
> > + *
> > + * Returns 0 on success, -EPERM on permission failure.
> > + *
> > + * Called (set) from fs/proc/base.c::proc_containerid_write().
> > + */
> > +int audit_set_containerid(struct task_struct *task, u64 containerid)
> > +{
> > + u64 oldcontainerid;
> > + int rc;
> > +
> > + oldcontainerid = audit_get_containerid(task);
> > +
> > + rc = audit_set_containerid_perm(task, containerid);
> > + if (!rc) {
> > + task_lock(task);
> > + task->containerid = containerid;
> > + task_unlock(task);
> > + }
> > +
> > + audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> > + return rc;
>
> Why are audit_set_containerid_perm() and audit_log_containerid()
> separate functions?
(I assume you mean audit_log_set_containerid()?)
It seemed clearer that all the permission checking was in one function
and its return code could be used to report the outcome when logging the
(attempted) action. This is the same structure as audit_set_loginuid()
and it made sense.
This would be the time to connect it to a syscall if that seems like a
good idea and remove pid, uid, auid, tty, ses fields.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-04-21 12:10 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, luto-DgEjT+Ai2ygdnm+yROfE0A,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, carlos-H+wXaHxf7aLQT0dZR+AlfA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
dhowells-H+wXaHxf7aLQT0dZR+AlfA, Linux-Audit Mailing List,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, simo-H+wXaHxf7aLQT0dZR+AlfA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Eric Paris
In-Reply-To: <20180420204225.iik2lgtj6gx2ep4w-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On April 20, 2018 4:48:34 PM Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
On 2018-04-20 16:22, Paul Moore wrote:
On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
On 2018-04-18 21:46, Paul Moore wrote:
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 container identifiiers.
Add/increment the container identifier on:
- initial setting of the container id via /proc
- clone/fork call that inherits a container identifier
- unshare call that inherits a container identifier
- setns call that inherits a container identifier
Delete/decrement the container identifier on:
- an inherited container id 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/32
See: https://github.com/linux-audit/audit-testsuite/issues/64
Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
include/linux/audit.h | 7 +++++++
include/net/net_namespace.h | 12 ++++++++++++
kernel/auditsc.c | 9 ++++++---
kernel/nsproxy.c | 6 ++++++
net/core/net_namespace.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 76 insertions(+), 3 deletions(-)
...
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index f6c5d33..d9f1090 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -140,6 +140,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 containerid = audit_get_containerid(tsk);
if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
CLONE_NEWPID | CLONE_NEWNET |
@@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
return PTR_ERR(new_ns);
tsk->nsproxy = new_ns;
+ net_add_audit_containerid(new_ns->net_ns, containerid);
return 0;
}
Hopefully we can handle this in audit_net_init(), we just need to
figure out where we can get the correct task_struct for the audit
container ID (some backpointer in the net struct?).
I don't follow. This needs to happen on every task startup.
audit_net_init() is only called when a new network namespace starts up.
Yep, sorry, my mistake. I must have confused myself when I was
looking at the code.
I'm thinking out loud here, bear with me ...
Assuming we move the netns/audit-container-ID tracking to audit_net,
and considering we already have an audit hook in copy_process() (it
calls audit_alloc()), would this be better handled by the
copy_process() hook? This ignores naming, audit_alloc() reuse, etc.;
those can be easily fixed. I'm just thinking of ways to limit our
impact on the core kernel and leverage our existing interaction
points.
The new namespace hasn't been cloned yet and this is the only function
where we have access to both namespaces, so I don't see how that could
work...
I'll take another, closer look, with v3.
paul moore
- RGB
--
Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
paul moore
www.paul-moore.com
^ permalink raw reply related
* Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
From: Richard Guy Briggs @ 2018-04-20 20:42 UTC (permalink / raw)
To: Paul Moore
Cc: simo, jlayton, carlos, linux-api, containers, LKML, Eric Paris,
dhowells, Linux-Audit Mailing List, ebiederm, luto, netdev,
linux-fsdevel, cgroups, serge, viro
In-Reply-To: <CAHC9VhTOYUAyCJidm99som6FVmjouQUGsEHarQ4h_NhwJxQQfw@mail.gmail.com>
On 2018-04-20 16:22, Paul Moore wrote:
> On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-04-18 21:46, Paul Moore wrote:
> >> On Fri, Mar 16, 2018 at 5:00 AM, 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 container identifiiers.
> >> >
> >> > Add/increment the container identifier on:
> >> > - initial setting of the container id via /proc
> >> > - clone/fork call that inherits a container identifier
> >> > - unshare call that inherits a container identifier
> >> > - setns call that inherits a container identifier
> >> > Delete/decrement the container identifier on:
> >> > - an inherited container id 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/32
> >> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> > include/linux/audit.h | 7 +++++++
> >> > include/net/net_namespace.h | 12 ++++++++++++
> >> > kernel/auditsc.c | 9 ++++++---
> >> > kernel/nsproxy.c | 6 ++++++
> >> > net/core/net_namespace.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >> > 5 files changed, 76 insertions(+), 3 deletions(-)
>
> ...
>
> >> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> >> > index f6c5d33..d9f1090 100644
> >> > --- a/kernel/nsproxy.c
> >> > +++ b/kernel/nsproxy.c
> >> > @@ -140,6 +140,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 containerid = audit_get_containerid(tsk);
> >> >
> >> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> >> > CLONE_NEWPID | CLONE_NEWNET |
> >> > @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> >> > return PTR_ERR(new_ns);
> >> >
> >> > tsk->nsproxy = new_ns;
> >> > + net_add_audit_containerid(new_ns->net_ns, containerid);
> >> > return 0;
> >> > }
> >>
> >> Hopefully we can handle this in audit_net_init(), we just need to
> >> figure out where we can get the correct task_struct for the audit
> >> container ID (some backpointer in the net struct?).
> >
> > I don't follow. This needs to happen on every task startup.
> > audit_net_init() is only called when a new network namespace starts up.
>
> Yep, sorry, my mistake. I must have confused myself when I was
> looking at the code.
>
> I'm thinking out loud here, bear with me ...
>
> Assuming we move the netns/audit-container-ID tracking to audit_net,
> and considering we already have an audit hook in copy_process() (it
> calls audit_alloc()), would this be better handled by the
> copy_process() hook? This ignores naming, audit_alloc() reuse, etc.;
> those can be easily fixed. I'm just thinking of ways to limit our
> impact on the core kernel and leverage our existing interaction
> points.
The new namespace hasn't been cloned yet and this is the only function
where we have access to both namespaces, so I don't see how that could
work...
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-04-20 20:22 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <20180420200226.7tyxzuovdbgclw3m@madcap2.tricolour.ca>
On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-18 21:46, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, 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 container identifiiers.
>> >
>> > Add/increment the container identifier on:
>> > - initial setting of the container id via /proc
>> > - clone/fork call that inherits a container identifier
>> > - unshare call that inherits a container identifier
>> > - setns call that inherits a container identifier
>> > Delete/decrement the container identifier on:
>> > - an inherited container id 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/32
>> > See: https://github.com/linux-audit/audit-testsuite/issues/64
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> > include/linux/audit.h | 7 +++++++
>> > include/net/net_namespace.h | 12 ++++++++++++
>> > kernel/auditsc.c | 9 ++++++---
>> > kernel/nsproxy.c | 6 ++++++
>> > net/core/net_namespace.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 76 insertions(+), 3 deletions(-)
...
>> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
>> > index f6c5d33..d9f1090 100644
>> > --- a/kernel/nsproxy.c
>> > +++ b/kernel/nsproxy.c
>> > @@ -140,6 +140,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 containerid = audit_get_containerid(tsk);
>> >
>> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
>> > CLONE_NEWPID | CLONE_NEWNET |
>> > @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
>> > return PTR_ERR(new_ns);
>> >
>> > tsk->nsproxy = new_ns;
>> > + net_add_audit_containerid(new_ns->net_ns, containerid);
>> > return 0;
>> > }
>>
>> Hopefully we can handle this in audit_net_init(), we just need to
>> figure out where we can get the correct task_struct for the audit
>> container ID (some backpointer in the net struct?).
>
> I don't follow. This needs to happen on every task startup.
> audit_net_init() is only called when a new network namespace starts up.
Yep, sorry, my mistake. I must have confused myself when I was
looking at the code.
I'm thinking out loud here, bear with me ...
Assuming we move the netns/audit-container-ID tracking to audit_net,
and considering we already have an audit hook in copy_process() (it
calls audit_alloc()), would this be better handled by the
copy_process() hook? This ignores naming, audit_alloc() reuse, etc.;
those can be easily fixed. I'm just thinking of ways to limit our
impact on the core kernel and leverage our existing interaction
points.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
From: Richard Guy Briggs @ 2018-04-20 20:02 UTC (permalink / raw)
To: Paul Moore
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhRkstDMjd5T3w+iOUDjzDAs1AOm0xd3p6v_xn6fNGYQhA@mail.gmail.com>
On 2018-04-18 21:46, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, 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 container identifiiers.
> >
> > Add/increment the container identifier on:
> > - initial setting of the container id via /proc
> > - clone/fork call that inherits a container identifier
> > - unshare call that inherits a container identifier
> > - setns call that inherits a container identifier
> > Delete/decrement the container identifier on:
> > - an inherited container id 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/32
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 7 +++++++
> > include/net/net_namespace.h | 12 ++++++++++++
> > kernel/auditsc.c | 9 ++++++---
> > kernel/nsproxy.c | 6 ++++++
> > net/core/net_namespace.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 76 insertions(+), 3 deletions(-)
>
> ...
>
> > diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> > index 0490084..343a428 100644
> > --- a/include/net/net_namespace.h
> > +++ b/include/net/net_namespace.h
> > @@ -33,6 +33,7 @@
> > #include <linux/ns_common.h>
> > #include <linux/idr.h>
> > #include <linux/skbuff.h>
> > +#include <linux/audit.h>
> >
> > struct user_namespace;
> > struct proc_dir_entry;
> > @@ -150,6 +151,7 @@ struct net {
> > #endif
> > struct sock *diag_nlsk;
> > atomic_t fnhe_genid;
> > + struct list_head audit_containerid;
> > } __randomize_layout;
>
> We talked about this briefly off-list, you should be using audit_net
> and the net_generic mechanism instead of this.
>
> > #include <linux/seq_file_net.h>
> > @@ -301,6 +303,16 @@ static inline struct net *read_pnet(const possible_net_t *pnet)
> > #define __net_initconst __initconst
> > #endif
> >
> > +#ifdef CONFIG_NET_NS
> > +void net_add_audit_containerid(struct net *net, u64 containerid);
> > +void net_del_audit_containerid(struct net *net, u64 containerid);
> > +#else
> > +static inline void net_add_audit_containerid(struct net *, u64)
> > +{ }
> > +static inline void net_del_audit_containerid(struct net *, u64)
> > +{ }
> > +#endif
> > +
> > int peernet2id_alloc(struct net *net, struct net *peer);
> > int peernet2id(struct net *net, struct net *peer);
> > bool peernet_has_id(struct net *net, struct net *peer);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 2f02ed9..208da962 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"
> >
> > @@ -2175,16 +2176,18 @@ static void audit_log_set_containerid(struct task_struct *task, u64 oldcontainer
> > */
> > int audit_set_containerid(struct task_struct *task, u64 containerid)
> > {
> > - u64 oldcontainerid;
> > + u64 oldcontainerid = audit_get_containerid(task);
> > int rc;
> > -
> > - oldcontainerid = audit_get_containerid(task);
> > + struct net *net = task->nsproxy->net_ns;
> >
> > rc = audit_set_containerid_perm(task, containerid);
> > if (!rc) {
> > + if (cid_valid(oldcontainerid))
> > + net_del_audit_containerid(net, oldcontainerid);
>
> Using audit_net we can handle this internal to audit, which is a Good Thing.
No problem, done.
> > task_lock(task);
> > task->containerid = containerid;
> > task_unlock(task);
> > + net_add_audit_containerid(net, containerid);
>
> Same.
>
> > }
> >
> > audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > index f6c5d33..d9f1090 100644
> > --- a/kernel/nsproxy.c
> > +++ b/kernel/nsproxy.c
> > @@ -140,6 +140,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 containerid = audit_get_containerid(tsk);
> >
> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> > CLONE_NEWPID | CLONE_NEWNET |
> > @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> > return PTR_ERR(new_ns);
> >
> > tsk->nsproxy = new_ns;
> > + net_add_audit_containerid(new_ns->net_ns, containerid);
> > return 0;
> > }
>
> Hopefully we can handle this in audit_net_init(), we just need to
> figure out where we can get the correct task_struct for the audit
> container ID (some backpointer in the net struct?).
I don't follow. This needs to happen on every task startup.
audit_net_init() is only called when a new network namespace starts up.
> > @@ -217,6 +219,7 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
> > void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> > {
> > struct nsproxy *ns;
> > + u64 containerid = audit_get_containerid(p);
> >
> > might_sleep();
> >
> > @@ -224,6 +227,9 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> > ns = p->nsproxy;
> > p->nsproxy = new;
> > task_unlock(p);
> > + net_del_audit_containerid(ns->net_ns, containerid);
> > + if (new)
> > + net_add_audit_containerid(new->net_ns, containerid);
>
> Okay, we might need a hook here for switching namespaces, but I would
> much rather it be a generic audit hook that calls directly into audit.
Trivial, done.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2018-04-20 16:21 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <20180420012346.udnga5pfdjoazcfc@madcap2.tricolour.ca>
On Thu, Apr 19, 2018 at 9:23 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-18 20:39, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, 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 | 20 +++++++++++++++++++-
>> > 2 files changed, 27 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/audit.h b/include/linux/audit.h
>> > index ed16bb6..c0b83cb 100644
>> > --- a/include/linux/audit.h
>> > +++ b/include/linux/audit.h
>> > @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct audit_context *context,
>> > /* These are defined in auditsc.c */
>> > /* Public API */
>> > extern int audit_alloc(struct task_struct *task);
>> > +extern struct audit_context *audit_alloc_local(void);
>> > extern void __audit_free(struct task_struct *task);
>> > +extern void audit_free_context(struct audit_context *context);
>> > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>> > unsigned long a2, unsigned long a3);
>> > extern void __audit_syscall_exit(int ret_success, long ret_value);
>> > @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct *task)
>> > {
>> > return 0;
>> > }
>> > +static inline struct audit_context *audit_alloc_local(void)
>> > +{
>> > + return NULL;
>> > +}
>> > +static inline void audit_free_context(struct audit_context *context)
>> > +{ }
>> > static inline void audit_free(struct task_struct *task)
>> > { }
>> > static inline void audit_syscall_entry(int major, unsigned long a0,
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 2932ef1..7103d23 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
>> > return 0;
>> > }
>> >
>> > -static inline void audit_free_context(struct audit_context *context)
>> > +struct audit_context *audit_alloc_local(void)
>> > {
>> > + 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;
>> > + return context;
>> > +}
>> > +
>> > +inline 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);
>>
>> I'm reserving the option to comment on this idea further as I make my
>> way through the patchset, but audit_free_context() definitely
>> shouldn't be declared as an inline function.
>
> Ok, I think I follow. When it wasn't exported, inline was fine, but now
> that it has been exported, it should no longer be inlined ...
Pretty much. Based on a few comments I've seen by compiler folks over
the years, my current thinking is that we shouldn't worry about
explicit inlining static functions in C files (header files are a
different story). The basic idea being that the compiler almost
always does a better job than us stupid developers.
> ... or should use
> an intermediate function name to export so that local uses of it can
> remain inline.
Possibly, but my guess is that the compiler could (will?) do that by
itself for code that lives in the same file.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
From: Paul Moore @ 2018-04-20 16:13 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <20180420010320.panie6mtdafxl65y@madcap2.tricolour.ca>
On Thu, Apr 19, 2018 at 9:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-18 20:32, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
...
>> > /*
>> > * audit_log_container_info - report container info
>> > - * @tsk: task to be recorded
>> > * @context: task or local context for record
>> > + * @op: containerid string description
>> > + * @containerid: container ID to report
>> > */
>> > -int audit_log_container_info(struct task_struct *tsk, struct audit_context *context)
>> > +int audit_log_container_info(struct audit_context *context,
>> > + char *op, u64 containerid)
>> > {
>> > struct audit_buffer *ab;
>> >
>> > - if (!audit_containerid_set(tsk))
>> > + if (!cid_valid(containerid))
>> > return 0;
>> > /* Generate AUDIT_CONTAINER_INFO with container ID */
>> > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO);
>> > if (!ab)
>> > return -ENOMEM;
>> > - audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk));
>> > + audit_log_format(ab, "op=%s contid=%llu", op, containerid);
>> > audit_log_end(ab);
>> > return 0;
>> > }
>>
>> Let's get these changes into the first patch where
>> audit_log_container_info() is defined. Why? This inserts a new field
>> into the record which is a no-no. Yes, it is one single patchset, but
>> they are still separate patches and who knows which patches a given
>> distribution and/or tree may decide to backport.
>
> Fair enough. That first thought went through my mind... Would it be
> sufficient to move that field addition to the first patch and leave the
> rest here to support trace and signals?
I should have been more clear ... yes, that's what I was thinking; the
record format is the important part as it's user visible.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
From: Paul Moore @ 2018-04-20 16:11 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: simo, jlayton, carlos, linux-api, containers, LKML, Eric Paris,
dhowells, Linux-Audit Mailing List, ebiederm, luto, netdev,
linux-fsdevel, cgroups, serge, viro
In-Reply-To: <20180420004218.tgndd474wgueyjzk@madcap2.tricolour.ca>
On Thu, Apr 19, 2018 at 8:42 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-18 21:31, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Add container ID auxiliary records to secure computing and abnormal end
>> > standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> > kernel/auditsc.c | 10 ++++++++--
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 7103d23..2f02ed9 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
>> > void audit_core_dumps(long signr)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> Looking quickly at do_coredump() I *believe* we can use current here.
>>
>> > if (!audit_enabled)
>> > return;
>> > @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
>> > if (signr == SIGQUIT) /* don't care for those */
>> > return;
>> >
>> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > if (unlikely(!ab))
>> > return;
>> > audit_log_task(ab);
>> > audit_log_format(ab, " sig=%ld res=1", signr);
>> > audit_log_end(ab);
>> > + audit_log_container_info(context, "abend", audit_get_containerid(current));
>> > + audit_free_context(context);
>> > }
>> >
>> > void __audit_seccomp(unsigned long syscall, long signr, int code)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> We can definitely use current here.
>
> Ok, so both syscall aux records. That elimintes this patch from the
> set, can go in independently.
Yep. It should help shrink the audit container ID patchset and
perhaps more importantly it should put some distance between the
connected-record debate and the audit container ID debate.
I understand we are going to need a "local" context for some things,
the network packets are probably the best example, but whenever
possible I would like to connect these records back to a task's
context.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] x86: ipc: fix x32 version of shmid64_ds and msqid64_ds
From: Arnd Bergmann @ 2018-04-20 14:38 UTC (permalink / raw)
To: Jeffrey Walton
Cc: open list:RALINK MIPS ARCHITECTURE, Adam Borowski, James Hogan,
Will Deacon, Albert ARIBAUD, Deepa Dinamani, H. Peter Anvin,
sparclinux, linux-arch, linux-s390, y2038 Mailman List,
the arch/x86 maintainers, Ingo Molnar, tg, Catalin Marinas,
Ben Hutchings, H . J . Lu, Daniel Schepler, Al Viro,
John Paul Adrian Glaubitz, Thomas Gleixner, GNU C Library, Linux
In-Reply-To: <CAH8yC8mnfNnG86kgjnfwiZJ0=qN+w=5PVcLcxddaJdDtYbSanA@mail.gmail.com>
On Fri, Apr 20, 2018 at 3:53 PM, Jeffrey Walton <noloader@gmail.com> wrote:
>> +#if !defined(__x86_64__) || !defined(__ilp32__)
>> #include <asm-generic/msgbuf.h>
>> +#else
>
> I understand there's some progress having Clang compile the kernel.
> Clang treats __ILP32__ and friends differently than GCC. I believe
> ILP32 shows up just about everywhere there are 32-bit ints, longs and
> pointers. You might find it on Aarch64 or you might find it on MIPS64
> when using Clang.
>
> I think that means this may be a little suspicious:
>
> > +#if !defined(__x86_64__) || !defined(__ilp32__)
>
> I kind of felt LLVM was wandering away from the x32 ABI, but the LLVM
> devs insisted they were within their purview. Also see
> https://lists.llvm.org/pipermail/cfe-dev/2015-December/046300.html.
>
> Sorry about the top-post. I just wanted to pick out that one piece.
It seems I made a typo and it needs to be __ILP32__ rather than
__ilp32__ (corrected that locally, will resend once we have resolved
this).
Aside from that, the #if check seems to be correct to me: this
is an x86-specific header, so it won't ever be seen on other
architectures. On x86-32, __x86_64__ isn't set, so we don't care
about whether __ilp32__ is set or not, and on x86-64 (lp64),
__ilp32__ is never set, so we still get the asm-generic header.
Arnd
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH] x86: ipc: fix x32 version of shmid64_ds and msqid64_ds
From: Jeffrey Walton @ 2018-04-20 13:53 UTC (permalink / raw)
To: Arnd Bergmann
Cc: x86, Thomas Gleixner, Ingo Molnar, ebiederm, y2038 Mailman List,
LKML, linux-api, linux-arch, libc-alpha, Deepa Dinamani, Al Viro,
Albert ARIBAUD, linux-s390, schwidefsky, Catalin Marinas,
Will Deacon, linux-mips, jhogan, Ralf Baechle, linuxppc-dev,
sparclinux, Ben Hutchings, Daniel Schepler, H . J . Lu
In-Reply-To: <20180420130346.3178914-1-arnd@arndb.de>
Hi Arnd,
One comment here:
> +#if !defined(__x86_64__) || !defined(__ilp32__)
> #include <asm-generic/msgbuf.h>
> +#else
I understand there's some progress having Clang compile the kernel.
Clang treats __ILP32__ and friends differently than GCC. I believe
ILP32 shows up just about everywhere there are 32-bit ints, longs and
pointers. You might find it on Aarch64 or you might find it on MIPS64
when using Clang.
I think that means this may be a little suspicious:
> +#if !defined(__x86_64__) || !defined(__ilp32__)
I kind of felt LLVM was wandering away from the x32 ABI, but the LLVM
devs insisted they were within their purview. Also see
https://lists.llvm.org/pipermail/cfe-dev/2015-December/046300.html.
Sorry about the top-post. I just wanted to pick out that one piece.
Jeff
On Fri, Apr 20, 2018 at 9:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> A bugfix broke the x32 shmid64_ds and msqid64_ds data structure layout
> (as seen from user space) a few years ago: Originally, __BITS_PER_LONG
> was defined as 64 on x32, so we did not have padding after the 64-bit
> __kernel_time_t fields, After __BITS_PER_LONG got changed to 32,
> applications would observe extra padding.
>
> In other parts of the uapi headers we seem to have a mix of those
> expecting either 32 or 64 on x32 applications, so we can't easily revert
> the path that broke these two structures.
>
> Instead, this patch decouples x32 from the other architectures and moves
> it back into arch specific headers, partially reverting the even older
> commit 73a2d096fdf2 ("x86: remove all now-duplicate header files").
>
> It's not clear whether this ever made any difference, since at least
> glibc carries its own (correct) copy of both of these header files,
> so possibly no application has ever observed the definitions here.
>
> There are other UAPI interfaces that depend on __BITS_PER_LONG and
> that might suffer from the same problem on x32, but I have not tried to
> analyse them in enough detail to be sure. If anyone still cares about x32,
> that may be a useful thing to do.
>
> Fixes: f4b4aae18288 ("x86/headers/uapi: Fix __BITS_PER_LONG value for x32 builds")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This came out of the y2038 ipc syscall series but can be applied
> and backported independently.
> ---
> arch/x86/include/uapi/asm/msgbuf.h | 29 +++++++++++++++++++++++++++
> arch/x86/include/uapi/asm/shmbuf.h | 40 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 69 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/msgbuf.h b/arch/x86/include/uapi/asm/msgbuf.h
> index 809134c644a6..5f1604961e6d 100644
> --- a/arch/x86/include/uapi/asm/msgbuf.h
> +++ b/arch/x86/include/uapi/asm/msgbuf.h
> @@ -1 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef __ASM_X64_MSGBUF_H
> +#define __ASM_X64_MSGBUF_H
> +
> +#if !defined(__x86_64__) || !defined(__ilp32__)
> #include <asm-generic/msgbuf.h>
> +#else
> +/*
> + * The msqid64_ds structure for x86 architecture with x32 ABI.
> + *
> + * On x86-32 and x86-64 we can just use the generic definition, but
> + * x32 uses the same binary layout as x86_64, which is differnet
> + * from other 32-bit architectures.
> + */
> +
> +struct msqid64_ds {
> + struct ipc64_perm msg_perm;
> + __kernel_time_t msg_stime; /* last msgsnd time */
> + __kernel_time_t msg_rtime; /* last msgrcv time */
> + __kernel_time_t msg_ctime; /* last change time */
> + __kernel_ulong_t msg_cbytes; /* current number of bytes on queue */
> + __kernel_ulong_t msg_qnum; /* number of messages in queue */
> + __kernel_ulong_t msg_qbytes; /* max number of bytes on queue */
> + __kernel_pid_t msg_lspid; /* pid of last msgsnd */
> + __kernel_pid_t msg_lrpid; /* last receive pid */
> + __kernel_ulong_t __unused4;
> + __kernel_ulong_t __unused5;
> +};
> +
> +#endif /* __ASM_GENERIC_MSGBUF_H */
> diff --git a/arch/x86/include/uapi/asm/shmbuf.h b/arch/x86/include/uapi/asm/shmbuf.h
> index 83c05fc2de38..cdd7eec878fa 100644
> --- a/arch/x86/include/uapi/asm/shmbuf.h
> +++ b/arch/x86/include/uapi/asm/shmbuf.h
> @@ -1 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef __ASM_X86_SHMBUF_H
> +#define __ASM_X86_SHMBUF_H
> +
> +#if !defined(__x86_64__) || !defined(__ilp32__)
> #include <asm-generic/shmbuf.h>
> +#else
> +/*
> + * The shmid64_ds structure for x86 architecture with x32 ABI.
> + *
> + * On x86-32 and x86-64 we can just use the generic definition, but
> + * x32 uses the same binary layout as x86_64, which is differnet
> + * from other 32-bit architectures.
> + */
> +
> +struct shmid64_ds {
> + struct ipc64_perm shm_perm; /* operation perms */
> + size_t shm_segsz; /* size of segment (bytes) */
> + __kernel_time_t shm_atime; /* last attach time */
> + __kernel_time_t shm_dtime; /* last detach time */
> + __kernel_time_t shm_ctime; /* last change time */
> + __kernel_pid_t shm_cpid; /* pid of creator */
> + __kernel_pid_t shm_lpid; /* pid of last operator */
> + __kernel_ulong_t shm_nattch; /* no. of current attaches */
> + __kernel_ulong_t __unused4;
> + __kernel_ulong_t __unused5;
> +};
> +
> +struct shminfo64 {
> + __kernel_ulong_t shmmax;
> + __kernel_ulong_t shmmin;
> + __kernel_ulong_t shmmni;
> + __kernel_ulong_t shmseg;
> + __kernel_ulong_t shmall;
> + __kernel_ulong_t __unused1;
> + __kernel_ulong_t __unused2;
> + __kernel_ulong_t __unused3;
> + __kernel_ulong_t __unused4;
> +};
> +
> +#endif /* __ASM_X86_SHMBUF_H */
^ permalink raw reply
* [PATCH] x86: ipc: fix x32 version of shmid64_ds and msqid64_ds
From: Arnd Bergmann @ 2018-04-20 13:03 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar
Cc: ebiederm, y2038 Mailman List, linux-kernel, linux-api, linux-arch,
libc-alpha, Deepa Dinamani, Al Viro, Albert ARIBAUD, linux-s390,
schwidefsky, Catalin Marinas, will.deacon, linux-mips, jhogan,
Ralf Baechle, linuxppc-dev, sparclinux, Ben Hutchings,
Jeffrey Walton, Daniel Schepler, H . J . Lu, Adam Borowski, tg,
John
In-Reply-To: <CAK8P3a3qAoR1afmTTK1CAp1L81dzwtBL+SKj=QMqD=dBr_8oRQ@mail.gmail.com>
A bugfix broke the x32 shmid64_ds and msqid64_ds data structure layout
(as seen from user space) a few years ago: Originally, __BITS_PER_LONG
was defined as 64 on x32, so we did not have padding after the 64-bit
__kernel_time_t fields, After __BITS_PER_LONG got changed to 32,
applications would observe extra padding.
In other parts of the uapi headers we seem to have a mix of those
expecting either 32 or 64 on x32 applications, so we can't easily revert
the path that broke these two structures.
Instead, this patch decouples x32 from the other architectures and moves
it back into arch specific headers, partially reverting the even older
commit 73a2d096fdf2 ("x86: remove all now-duplicate header files").
It's not clear whether this ever made any difference, since at least
glibc carries its own (correct) copy of both of these header files,
so possibly no application has ever observed the definitions here.
There are other UAPI interfaces that depend on __BITS_PER_LONG and
that might suffer from the same problem on x32, but I have not tried to
analyse them in enough detail to be sure. If anyone still cares about x32,
that may be a useful thing to do.
Fixes: f4b4aae18288 ("x86/headers/uapi: Fix __BITS_PER_LONG value for x32 builds")
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This came out of the y2038 ipc syscall series but can be applied
and backported independently.
---
arch/x86/include/uapi/asm/msgbuf.h | 29 +++++++++++++++++++++++++++
arch/x86/include/uapi/asm/shmbuf.h | 40 ++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
diff --git a/arch/x86/include/uapi/asm/msgbuf.h b/arch/x86/include/uapi/asm/msgbuf.h
index 809134c644a6..5f1604961e6d 100644
--- a/arch/x86/include/uapi/asm/msgbuf.h
+++ b/arch/x86/include/uapi/asm/msgbuf.h
@@ -1 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_X64_MSGBUF_H
+#define __ASM_X64_MSGBUF_H
+
+#if !defined(__x86_64__) || !defined(__ilp32__)
#include <asm-generic/msgbuf.h>
+#else
+/*
+ * The msqid64_ds structure for x86 architecture with x32 ABI.
+ *
+ * On x86-32 and x86-64 we can just use the generic definition, but
+ * x32 uses the same binary layout as x86_64, which is differnet
+ * from other 32-bit architectures.
+ */
+
+struct msqid64_ds {
+ struct ipc64_perm msg_perm;
+ __kernel_time_t msg_stime; /* last msgsnd time */
+ __kernel_time_t msg_rtime; /* last msgrcv time */
+ __kernel_time_t msg_ctime; /* last change time */
+ __kernel_ulong_t msg_cbytes; /* current number of bytes on queue */
+ __kernel_ulong_t msg_qnum; /* number of messages in queue */
+ __kernel_ulong_t msg_qbytes; /* max number of bytes on queue */
+ __kernel_pid_t msg_lspid; /* pid of last msgsnd */
+ __kernel_pid_t msg_lrpid; /* last receive pid */
+ __kernel_ulong_t __unused4;
+ __kernel_ulong_t __unused5;
+};
+
+#endif /* __ASM_GENERIC_MSGBUF_H */
diff --git a/arch/x86/include/uapi/asm/shmbuf.h b/arch/x86/include/uapi/asm/shmbuf.h
index 83c05fc2de38..cdd7eec878fa 100644
--- a/arch/x86/include/uapi/asm/shmbuf.h
+++ b/arch/x86/include/uapi/asm/shmbuf.h
@@ -1 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_X86_SHMBUF_H
+#define __ASM_X86_SHMBUF_H
+
+#if !defined(__x86_64__) || !defined(__ilp32__)
#include <asm-generic/shmbuf.h>
+#else
+/*
+ * The shmid64_ds structure for x86 architecture with x32 ABI.
+ *
+ * On x86-32 and x86-64 we can just use the generic definition, but
+ * x32 uses the same binary layout as x86_64, which is differnet
+ * from other 32-bit architectures.
+ */
+
+struct shmid64_ds {
+ struct ipc64_perm shm_perm; /* operation perms */
+ size_t shm_segsz; /* size of segment (bytes) */
+ __kernel_time_t shm_atime; /* last attach time */
+ __kernel_time_t shm_dtime; /* last detach time */
+ __kernel_time_t shm_ctime; /* last change time */
+ __kernel_pid_t shm_cpid; /* pid of creator */
+ __kernel_pid_t shm_lpid; /* pid of last operator */
+ __kernel_ulong_t shm_nattch; /* no. of current attaches */
+ __kernel_ulong_t __unused4;
+ __kernel_ulong_t __unused5;
+};
+
+struct shminfo64 {
+ __kernel_ulong_t shmmax;
+ __kernel_ulong_t shmmin;
+ __kernel_ulong_t shmmni;
+ __kernel_ulong_t shmseg;
+ __kernel_ulong_t shmall;
+ __kernel_ulong_t __unused1;
+ __kernel_ulong_t __unused2;
+ __kernel_ulong_t __unused3;
+ __kernel_ulong_t __unused4;
+};
+
+#endif /* __ASM_X86_SHMBUF_H */
--
2.9.0
^ permalink raw reply related
* [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Arnd Bergmann @ 2018-04-20 12:05 UTC (permalink / raw)
To: y2038
Cc: linux-kernel, x86, linux-api, linux-arch, Arnd Bergmann,
Paul Eggert, Eric W . Biederman, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Al Viro, Dominik Brodowski,
Thomas Gleixner, Andrew Morton, linux-alpha, Deepa Dinamani,
Ingo Molnar
In-Reply-To: <20180420120605.1612248-1-arnd@arndb.de>
As part of the y2038 system call work, the getrusage/wait4/waitid system
calls came up, since they pass time data to user space in 'timeval'
format as required by POSIX. This means the existing kernel data structure
is no longer compatible with the one from user space once we have a C
library exposing 64-bit time_t, requiring the library to convert the
kernel structure into its own structure.
This patch moves that conversion into the kernel itself, providing a
set of system calls that can directly be used to implement the libc
getrusage/wait4/waitid functions as we have traditionally done.
There are two advantages to this:
- The new path becomes the native case, avoiding the conversion
overhead for future 32-bit C libraries. At least glibc will still
have to implement a conversion logic as a fallback in order to run
new applications on older kernels, but that does not have to be used
on new kernels.
- The range for the ru_utime/ru_stime is no longer limited to
a 31-bit second counter (about 68 years). That limit may theoretically
be hit on large SMP systems with a single process running for an
extended time, e.g. 256 concurrent threads running for more than
97 days. Note that there is no overflow in 2038, as all the times
are relative to the start of a process.
The downside of this is obviously the added complexity of having three
additional system call entry points plus their respective compat handlers,
and updated syscall tables on each architecture (not included in this
patch). Overall, I think this is *not* worth it, but I feel it's important
to show how it can be done and what the cost is.
There are probably some minor improvements that can be implemented on top,
as well as bugs that I introduce. When reviewing this patch, let's for
now focus instead on the question whether we want it at all or not.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/alpha/kernel/osf_sys.c | 2 +-
include/linux/compat.h | 26 ++++++++-
include/linux/resource.h | 4 +-
include/linux/sched/task.h | 4 +-
include/linux/syscalls.h | 8 +++
include/uapi/linux/resource.h | 29 ++++++++++
kernel/compat.c | 30 ++++++++++-
kernel/exit.c | 120 ++++++++++++++++++++++++++++++++++++++----
kernel/sys.c | 74 +++++++++++++++++++++++---
9 files changed, 275 insertions(+), 22 deletions(-)
diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index cad03ee445b3..aecdb48257b5 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1185,7 +1185,7 @@ SYSCALL_DEFINE4(osf_wait4, pid_t, pid, int __user *, ustatus, int, options,
{
unsigned int status = 0;
struct rusage32 r32;
- struct rusage r;
+ struct __kernel_rusage r;
long err = kernel_wait4(pid, &status, options, &r);
if (err <= 0)
return err;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index b73e2616a409..2ef30d314c48 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -105,7 +105,7 @@ typedef __compat_uid32_t compat_uid_t;
typedef __compat_gid32_t compat_gid_t;
struct compat_sel_arg_struct;
-struct rusage;
+struct __kernel_rusage;
struct compat_itimerspec {
struct compat_timespec it_interval;
@@ -321,9 +321,31 @@ struct compat_rusage {
compat_long_t ru_nivcsw;
};
-extern int put_compat_rusage(const struct rusage *,
+struct compat_rusage_time64 {
+ struct __kernel_rusage_timeval ru_utime;
+ struct __kernel_rusage_timeval ru_stime;
+ compat_long_t ru_maxrss;
+ compat_long_t ru_ixrss;
+ compat_long_t ru_idrss;
+ compat_long_t ru_isrss;
+ compat_long_t ru_minflt;
+ compat_long_t ru_majflt;
+ compat_long_t ru_nswap;
+ compat_long_t ru_inblock;
+ compat_long_t ru_oublock;
+ compat_long_t ru_msgsnd;
+ compat_long_t ru_msgrcv;
+ compat_long_t ru_nsignals;
+ compat_long_t ru_nvcsw;
+ compat_long_t ru_nivcsw;
+};
+
+extern int put_compat_rusage(const struct __kernel_rusage *,
struct compat_rusage __user *);
+extern int put_compat_rusage_time64(const struct __kernel_rusage *,
+ struct compat_rusage_time64 __user *);
+
struct compat_siginfo;
struct compat_dirent {
diff --git a/include/linux/resource.h b/include/linux/resource.h
index bdf491cbcab7..8cebf90e76b7 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -7,8 +7,10 @@
struct task_struct;
-void getrusage(struct task_struct *p, int who, struct rusage *ru);
+void getrusage(struct task_struct *p, int who, struct __kernel_rusage *ru);
int do_prlimit(struct task_struct *tsk, unsigned int resource,
struct rlimit *new_rlim, struct rlimit *old_rlim);
+int put_rusage(const struct __kernel_rusage *rk, struct rusage __user *ru);
+
#endif
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index 5be31eb7b266..cc54ae5e6010 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -10,7 +10,7 @@
#include <linux/sched.h>
struct task_struct;
-struct rusage;
+struct __kernel_rusage;
union thread_union;
/*
@@ -75,7 +75,7 @@ extern long _do_fork(unsigned long, unsigned long, unsigned long, int __user *,
extern long do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *);
struct task_struct *fork_idle(int);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
-extern long kernel_wait4(pid_t, int *, int, struct rusage *);
+extern long kernel_wait4(pid_t, int *, int, struct __kernel_rusage *);
extern void free_task(struct task_struct *tsk);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index a756ab42894f..084360078f29 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -37,6 +37,7 @@ struct pollfd;
struct rlimit;
struct rlimit64;
struct rusage;
+struct __kernel_rusage;
struct sched_param;
struct sched_attr;
struct sel_arg_struct;
@@ -522,6 +523,10 @@ asmlinkage long sys_waitid(int which, pid_t pid,
struct siginfo __user *infop,
int options, struct rusage __user *ru);
+asmlinkage long sys_waitid_time64(int which, pid_t pid,
+ struct siginfo __user *infop,
+ int options, struct __kernel_rusage __user *ru);
+
/* kernel/fork.c */
asmlinkage long sys_set_tid_address(int __user *tidptr);
asmlinkage long sys_unshare(unsigned long unshare_flags);
@@ -656,6 +661,7 @@ asmlinkage long sys_getrlimit(unsigned int resource,
asmlinkage long sys_setrlimit(unsigned int resource,
struct rlimit __user *rlim);
asmlinkage long sys_getrusage(int who, struct rusage __user *ru);
+asmlinkage long sys_getrusage_time64(int who, struct __kernel_rusage __user *ru);
asmlinkage long sys_umask(int mask);
asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
@@ -821,6 +827,8 @@ asmlinkage long sys_recvmmsg(int fd, struct mmsghdr __user *msg,
asmlinkage long sys_wait4(pid_t pid, int __user *stat_addr,
int options, struct rusage __user *ru);
+asmlinkage long sys_wait4_time64(pid_t pid, int __user *stat_addr,
+ int options, struct __kernel_rusage __user *ru);
asmlinkage long sys_prlimit64(pid_t pid, unsigned int resource,
const struct rlimit64 __user *new_rlim,
struct rlimit64 __user *old_rlim);
diff --git a/include/uapi/linux/resource.h b/include/uapi/linux/resource.h
index 611d3745c70a..a822e716e122 100644
--- a/include/uapi/linux/resource.h
+++ b/include/uapi/linux/resource.h
@@ -50,6 +50,35 @@ struct rusage {
__kernel_long_t ru_nivcsw; /* involuntary " */
};
+/*
+ * __kernel_rusage is the binary that we expect 32-bit C libraries
+ * to provide for their 'struct rusage' after migrating to a 64-bit
+ * time_t.
+ */
+struct __kernel_rusage_timeval {
+ __s64 tv_sec;
+ __s64 tv_usec;
+};
+
+struct __kernel_rusage {
+ struct __kernel_rusage_timeval ru_utime; /* user time used */
+ struct __kernel_rusage_timeval ru_stime; /* system time used */
+ __kernel_long_t ru_maxrss; /* maximum resident set size */
+ __kernel_long_t ru_ixrss; /* integral shared memory size */
+ __kernel_long_t ru_idrss; /* integral unshared data size */
+ __kernel_long_t ru_isrss; /* integral unshared stack size */
+ __kernel_long_t ru_minflt; /* page reclaims */
+ __kernel_long_t ru_majflt; /* page faults */
+ __kernel_long_t ru_nswap; /* swaps */
+ __kernel_long_t ru_inblock; /* block input operations */
+ __kernel_long_t ru_oublock; /* block output operations */
+ __kernel_long_t ru_msgsnd; /* messages sent */
+ __kernel_long_t ru_msgrcv; /* messages received */
+ __kernel_long_t ru_nsignals; /* signals received */
+ __kernel_long_t ru_nvcsw; /* voluntary context switches */
+ __kernel_long_t ru_nivcsw; /* involuntary " */
+};
+
struct rlimit {
__kernel_ulong_t rlim_cur;
__kernel_ulong_t rlim_max;
diff --git a/kernel/compat.c b/kernel/compat.c
index 51a081b46832..e3cb7c14558a 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -234,7 +234,7 @@ COMPAT_SYSCALL_DEFINE3(sigprocmask, int, how,
#endif
-int put_compat_rusage(const struct rusage *r, struct compat_rusage __user *ru)
+int put_compat_rusage(const struct __kernel_rusage *r, struct compat_rusage __user *ru)
{
struct compat_rusage r32;
memset(&r32, 0, sizeof(r32));
@@ -261,6 +261,34 @@ int put_compat_rusage(const struct rusage *r, struct compat_rusage __user *ru)
return 0;
}
+int put_compat_rusage_time64(const struct __kernel_rusage *r,
+ struct compat_rusage_time64 __user *ru)
+{
+ struct compat_rusage_time64 r32;
+ memset(&r32, 0, sizeof(r32));
+ r32.ru_utime.tv_sec = r->ru_utime.tv_sec;
+ r32.ru_utime.tv_usec = r->ru_utime.tv_usec;
+ r32.ru_stime.tv_sec = r->ru_stime.tv_sec;
+ r32.ru_stime.tv_usec = r->ru_stime.tv_usec;
+ r32.ru_maxrss = r->ru_maxrss;
+ r32.ru_ixrss = r->ru_ixrss;
+ r32.ru_idrss = r->ru_idrss;
+ r32.ru_isrss = r->ru_isrss;
+ r32.ru_minflt = r->ru_minflt;
+ r32.ru_majflt = r->ru_majflt;
+ r32.ru_nswap = r->ru_nswap;
+ r32.ru_inblock = r->ru_inblock;
+ r32.ru_oublock = r->ru_oublock;
+ r32.ru_msgsnd = r->ru_msgsnd;
+ r32.ru_msgrcv = r->ru_msgrcv;
+ r32.ru_nsignals = r->ru_nsignals;
+ r32.ru_nvcsw = r->ru_nvcsw;
+ r32.ru_nivcsw = r->ru_nivcsw;
+ if (copy_to_user(ru, &r32, sizeof(r32)))
+ return -EFAULT;
+ return 0;
+}
+
static int compat_get_user_cpu_mask(compat_ulong_t __user *user_mask_ptr,
unsigned len, struct cpumask *new_mask)
{
diff --git a/kernel/exit.c b/kernel/exit.c
index c3c7ac560114..5088c671ea74 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -995,7 +995,7 @@ struct wait_opts {
struct waitid_info *wo_info;
int wo_stat;
- struct rusage *wo_rusage;
+ struct __kernel_rusage *wo_rusage;
wait_queue_entry_t child_wait;
int notask_error;
@@ -1548,7 +1548,7 @@ static long do_wait(struct wait_opts *wo)
}
static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
- int options, struct rusage *ru)
+ int options, struct __kernel_rusage *ru)
{
struct wait_opts wo;
struct pid *pid = NULL;
@@ -1596,7 +1596,7 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
infop, int, options, struct rusage __user *, ru)
{
- struct rusage r;
+ struct __kernel_rusage r;
struct waitid_info info = {.status = 0};
long err = kernel_waitid(which, upid, &info, options, ru ? &r : NULL);
int signo = 0;
@@ -1604,7 +1604,41 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
if (err > 0) {
signo = SIGCHLD;
err = 0;
- if (ru && copy_to_user(ru, &r, sizeof(struct rusage)))
+ if (ru && put_rusage(&r, ru))
+ return -EFAULT;
+ }
+ if (!infop)
+ return err;
+
+ if (!access_ok(VERIFY_WRITE, infop, sizeof(*infop)))
+ return -EFAULT;
+
+ user_access_begin();
+ unsafe_put_user(signo, &infop->si_signo, Efault);
+ unsafe_put_user(0, &infop->si_errno, Efault);
+ unsafe_put_user(info.cause, &infop->si_code, Efault);
+ unsafe_put_user(info.pid, &infop->si_pid, Efault);
+ unsafe_put_user(info.uid, &infop->si_uid, Efault);
+ unsafe_put_user(info.status, &infop->si_status, Efault);
+ user_access_end();
+ return err;
+Efault:
+ user_access_end();
+ return -EFAULT;
+}
+
+SYSCALL_DEFINE5(waitid_time64, int, which, pid_t, upid, struct siginfo __user *,
+ infop, int, options, struct __kernel_rusage __user *, ru)
+{
+ struct __kernel_rusage r;
+ struct waitid_info info = {.status = 0};
+ long err = kernel_waitid(which, upid, &info, options, ru ? &r : NULL);
+ int signo = 0;
+
+ if (err > 0) {
+ signo = SIGCHLD;
+ err = 0;
+ if (ru && copy_to_user(ru, &r, sizeof(struct __kernel_rusage)))
return -EFAULT;
}
if (!infop)
@@ -1628,7 +1662,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
}
long kernel_wait4(pid_t upid, int __user *stat_addr, int options,
- struct rusage *ru)
+ struct __kernel_rusage *ru)
{
struct wait_opts wo;
struct pid *pid = NULL;
@@ -1673,11 +1707,24 @@ long kernel_wait4(pid_t upid, int __user *stat_addr, int options,
SYSCALL_DEFINE4(wait4, pid_t, upid, int __user *, stat_addr,
int, options, struct rusage __user *, ru)
{
- struct rusage r;
+ struct __kernel_rusage r;
long err = kernel_wait4(upid, stat_addr, options, ru ? &r : NULL);
if (err > 0) {
- if (ru && copy_to_user(ru, &r, sizeof(struct rusage)))
+ if (ru && put_rusage(&r, ru))
+ return -EFAULT;
+ }
+ return err;
+}
+
+SYSCALL_DEFINE4(wait4_time64, pid_t, upid, int __user *, stat_addr,
+ int, options, struct __kernel_rusage __user *, ru)
+{
+ struct __kernel_rusage r;
+ long err = kernel_wait4(upid, stat_addr, options, ru ? &r : NULL);
+
+ if (err > 0) {
+ if (ru && copy_to_user(ru, &r, sizeof(struct __kernel_rusage)))
return -EFAULT;
}
return err;
@@ -1703,7 +1750,7 @@ COMPAT_SYSCALL_DEFINE4(wait4,
int, options,
struct compat_rusage __user *, ru)
{
- struct rusage r;
+ struct __kernel_rusage r;
long err = kernel_wait4(pid, stat_addr, options, ru ? &r : NULL);
if (err > 0) {
if (ru && put_compat_rusage(&r, ru))
@@ -1712,12 +1759,27 @@ COMPAT_SYSCALL_DEFINE4(wait4,
return err;
}
+COMPAT_SYSCALL_DEFINE4(wait4_time64,
+ compat_pid_t, pid,
+ compat_uint_t __user *, stat_addr,
+ int, options,
+ struct compat_rusage_time64 __user *, ru)
+{
+ struct __kernel_rusage r;
+ long err = kernel_wait4(pid, stat_addr, options, ru ? &r : NULL);
+ if (err > 0) {
+ if (ru && put_compat_rusage_time64(&r, ru))
+ return -EFAULT;
+ }
+ return err;
+}
+
COMPAT_SYSCALL_DEFINE5(waitid,
int, which, compat_pid_t, pid,
struct compat_siginfo __user *, infop, int, options,
struct compat_rusage __user *, uru)
{
- struct rusage ru;
+ struct __kernel_rusage ru;
struct waitid_info info = {.status = 0};
long err = kernel_waitid(which, pid, &info, options, uru ? &ru : NULL);
int signo = 0;
@@ -1754,6 +1816,46 @@ COMPAT_SYSCALL_DEFINE5(waitid,
user_access_end();
return -EFAULT;
}
+
+COMPAT_SYSCALL_DEFINE5(waitid_time64,
+ int, which, compat_pid_t, pid,
+ struct compat_siginfo __user *, infop, int, options,
+ struct compat_rusage_time64 __user *, uru)
+{
+ struct __kernel_rusage ru;
+ struct waitid_info info = {.status = 0};
+ long err = kernel_waitid(which, pid, &info, options, uru ? &ru : NULL);
+ int signo = 0;
+ if (err > 0) {
+ signo = SIGCHLD;
+ err = 0;
+ if (uru) {
+ /* kernel_waitid() overwrites everything in ru */
+ err = put_compat_rusage_time64(&ru, uru);
+ if (err)
+ return -EFAULT;
+ }
+ }
+
+ if (!infop)
+ return err;
+
+ if (!access_ok(VERIFY_WRITE, infop, sizeof(*infop)))
+ return -EFAULT;
+
+ user_access_begin();
+ unsafe_put_user(signo, &infop->si_signo, Efault);
+ unsafe_put_user(0, &infop->si_errno, Efault);
+ unsafe_put_user(info.cause, &infop->si_code, Efault);
+ unsafe_put_user(info.pid, &infop->si_pid, Efault);
+ unsafe_put_user(info.uid, &infop->si_uid, Efault);
+ unsafe_put_user(info.status, &infop->si_status, Efault);
+ user_access_end();
+ return err;
+Efault:
+ user_access_end();
+ return -EFAULT;
+}
#endif
__weak void abort(void)
diff --git a/kernel/sys.c b/kernel/sys.c
index 1de538f622e8..5b5f2dc19e79 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1699,7 +1699,7 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user *, rlim)
*
*/
-static void accumulate_thread_rusage(struct task_struct *t, struct rusage *r)
+static void accumulate_thread_rusage(struct task_struct *t, struct __kernel_rusage *r)
{
r->ru_nvcsw += t->nvcsw;
r->ru_nivcsw += t->nivcsw;
@@ -1709,12 +1709,13 @@ static void accumulate_thread_rusage(struct task_struct *t, struct rusage *r)
r->ru_oublock += task_io_get_oublock(t);
}
-void getrusage(struct task_struct *p, int who, struct rusage *r)
+void getrusage(struct task_struct *p, int who, struct __kernel_rusage *r)
{
struct task_struct *t;
unsigned long flags;
u64 tgutime, tgstime, utime, stime;
unsigned long maxrss = 0;
+ struct timespec64 ts;
memset((char *)r, 0, sizeof (*r));
utime = stime = 0;
@@ -1769,8 +1770,12 @@ void getrusage(struct task_struct *p, int who, struct rusage *r)
unlock_task_sighand(p, &flags);
out:
- r->ru_utime = ns_to_kernel_old_timeval(utime);
- r->ru_stime = ns_to_kernel_old_timeval(stime);
+ ts = ns_to_timespec64(utime);
+ r->ru_utime.tv_sec = ts.tv_sec;
+ r->ru_utime.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+ ts = ns_to_timespec64(stime);
+ r->ru_stime.tv_sec = ts.tv_sec;
+ r->ru_stime.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
if (who != RUSAGE_CHILDREN) {
struct mm_struct *mm = get_task_mm(p);
@@ -1783,10 +1788,54 @@ void getrusage(struct task_struct *p, int who, struct rusage *r)
r->ru_maxrss = maxrss * (PAGE_SIZE / 1024); /* convert pages to KBs */
}
-SYSCALL_DEFINE2(getrusage, int, who, struct rusage __user *, ru)
+int put_rusage(const struct __kernel_rusage *rk, struct rusage __user *ru)
{
struct rusage r;
+ if (IS_ENABLED(CONFIG_64BIT))
+ return copy_to_user(ru, &rk, sizeof(rk)) ? -EFAULT : 0;
+
+ memset(&r, 0, sizeof(r));
+ r.ru_utime.tv_sec = rk->ru_utime.tv_sec;
+ r.ru_utime.tv_usec = rk->ru_utime.tv_usec;
+ r.ru_stime.tv_sec = rk->ru_stime.tv_sec;
+ r.ru_stime.tv_usec = rk->ru_stime.tv_usec;
+ r.ru_maxrss = rk->ru_maxrss;
+ r.ru_ixrss = rk->ru_ixrss;
+ r.ru_idrss = rk->ru_idrss;
+ r.ru_isrss = rk->ru_isrss;
+ r.ru_minflt = rk->ru_minflt;
+ r.ru_majflt = rk->ru_majflt;
+ r.ru_nswap = rk->ru_nswap;
+ r.ru_inblock = rk->ru_inblock;
+ r.ru_oublock = rk->ru_oublock;
+ r.ru_msgsnd = rk->ru_msgsnd;
+ r.ru_msgrcv = rk->ru_msgrcv;
+ r.ru_nsignals = rk->ru_nsignals;
+ r.ru_nvcsw = rk->ru_nvcsw;
+ r.ru_nivcsw = rk->ru_nivcsw;
+ if (copy_to_user(ru, &r, sizeof(r)))
+ return -EFAULT;
+ return 0;
+}
+
+SYSCALL_DEFINE2(getrusage, int, who, struct rusage __user *, ru)
+{
+ struct __kernel_rusage r;
+
+ if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN &&
+ who != RUSAGE_THREAD)
+ return -EINVAL;
+
+ getrusage(current, who, &r);
+ return put_rusage(&r, ru);
+}
+
+#ifndef CONFIG_64BIT
+SYSCALL_DEFINE2(getrusage_time64, int, who, struct __kernel_rusage __user *, ru)
+{
+ struct __kernel_rusage r;
+
if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN &&
who != RUSAGE_THREAD)
return -EINVAL;
@@ -1794,11 +1843,12 @@ SYSCALL_DEFINE2(getrusage, int, who, struct rusage __user *, ru)
getrusage(current, who, &r);
return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
}
+#endif
#ifdef CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE2(getrusage, int, who, struct compat_rusage __user *, ru)
{
- struct rusage r;
+ struct __kernel_rusage r;
if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN &&
who != RUSAGE_THREAD)
@@ -1807,6 +1857,18 @@ COMPAT_SYSCALL_DEFINE2(getrusage, int, who, struct compat_rusage __user *, ru)
getrusage(current, who, &r);
return put_compat_rusage(&r, ru);
}
+
+COMPAT_SYSCALL_DEFINE2(getrusage_time64, int, who, struct compat_rusage_time64 __user *, ru)
+{
+ struct __kernel_rusage r;
+
+ if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN &&
+ who != RUSAGE_THREAD)
+ return -EINVAL;
+
+ getrusage(current, who, &r);
+ return put_compat_rusage_time64(&r, ru);
+}
#endif
SYSCALL_DEFINE1(umask, int, mask)
--
2.9.0
^ permalink raw reply related
* [PATCH v2 1/2] y2038: rusage: Use __kernel_old_timeval for process times
From: Arnd Bergmann @ 2018-04-20 12:05 UTC (permalink / raw)
To: y2038
Cc: linux-kernel, x86, linux-api, linux-arch, Arnd Bergmann,
Paul Eggert, Eric W . Biederman, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Al Viro, Dominik Brodowski,
Thomas Gleixner, Andrew Morton, linux-alpha
'struct rusage' contains the run times of a process in 'timeval' format
and is accessed through the wait4() and getrusage() system calls. This
is not a problem for y2038 safety by itself, but causes an issue when
the C library starts using 64-bit time_t on 32-bit architectures because
the structure layout becomes incompatible.
There are three possible ways of dealing with this:
a) deprecate the wait4() and getrusage() system calls, and create
a set of kernel interfaces based around a newly defined structure that
could solve multiple problems at once, e.g. provide more fine-grained
timestamps. The C library could then implement the posix interfaces
on top of the new system calls.
b) Extend the approach taken by the x32 ABI, and use the 64-bit
native structure layout for rusage on all architectures with new
system calls that is otherwise compatible. A downside of this
is that it requires a number of ugly hacks to deal with all the
other fields of the structure also becoming 64 bit wide.
Especially on big-endian architectures, we can't easily use the
union trick from glibc.
c) Change the definition of struct rusage to be independent of
time_t. This is the easiest change, as it does not involve new system
call entry points, but it requires the C library to convert between
the kernel format of the structure and the user space definition.
d) Add a new ABI variant of 'struct rusage' that corresponds to the
current layout with 32-bit counters but 64-bit time_t. This would
minimize the libc changes but require additional kernel code to
handle a third binary layout on 64-bit kernels.
I'm picking approach c) for its simplicity. As pointed out by reviewers,
simply using the kernel structure in user space would not be POSIX
compliant, but I have verified that none of the usual C libraries (glibc,
musl, uclibc-ng, newlib) do that. Instead, they all provide their own
definition of 'struct rusage' to applications in sys/resource.h.
To be on the safe side, I'm only changing the definition inside of
the kernel and for user space with an updated 'time_t'. All existing
users will see the traditional layout that is compatible with what the
C libraries export. A 32-bit application that includes linux/resource.h
but uses an update C library with 64-bit time_t will now see the low-level
kernel structure that corresponds to the getrusage() system call interface
but that will be different from one defined in sys/resource.h for the
getrusage library interface.
Link: https://patchwork.kernel.org/patch/10077527/
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/alpha/kernel/osf_sys.c | 15 +++++++++------
include/uapi/linux/resource.h | 14 ++++++++++++--
kernel/sys.c | 4 ++--
3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 89faa6f4de47..cad03ee445b3 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1184,6 +1184,7 @@ SYSCALL_DEFINE4(osf_wait4, pid_t, pid, int __user *, ustatus, int, options,
struct rusage32 __user *, ur)
{
unsigned int status = 0;
+ struct rusage32 r32;
struct rusage r;
long err = kernel_wait4(pid, &status, options, &r);
if (err <= 0)
@@ -1192,12 +1193,14 @@ SYSCALL_DEFINE4(osf_wait4, pid_t, pid, int __user *, ustatus, int, options,
return -EFAULT;
if (!ur)
return err;
- if (put_tv_to_tv32(&ur->ru_utime, &r.ru_utime))
- return -EFAULT;
- if (put_tv_to_tv32(&ur->ru_stime, &r.ru_stime))
- return -EFAULT;
- if (copy_to_user(&ur->ru_maxrss, &r.ru_maxrss,
- sizeof(struct rusage32) - offsetof(struct rusage32, ru_maxrss)))
+ r32.ru_utime.tv_sec = r.ru_utime.tv_sec;
+ r32.ru_utime.tv_usec = r.ru_utime.tv_usec;
+ r32.ru_stime.tv_sec = r.ru_stime.tv_sec;
+ r32.ru_stime.tv_usec = r.ru_stime.tv_usec;
+ memcpy(&r32.ru_maxrss, &r.ru_maxrss,
+ sizeof(struct rusage32) - offsetof(struct rusage32, ru_maxrss));
+
+ if (copy_to_user(ur, &r32, sizeof(r32)))
return -EFAULT;
return err;
}
diff --git a/include/uapi/linux/resource.h b/include/uapi/linux/resource.h
index cc00fd079631..611d3745c70a 100644
--- a/include/uapi/linux/resource.h
+++ b/include/uapi/linux/resource.h
@@ -22,8 +22,18 @@
#define RUSAGE_THREAD 1 /* only the calling thread */
struct rusage {
- struct timeval ru_utime; /* user time used */
- struct timeval ru_stime; /* system time used */
+#if (__BITS_PER_LONG != 32 || !defined(__USE_TIME_BITS64)) && !defined(__KERNEL__)
+ struct timeval ru_utime; /* user time used */
+ struct timeval ru_stime; /* system time used */
+#else
+ /*
+ * For 32-bit user space with 64-bit time_t, the binary layout
+ * in these fields is incompatible with 'struct timeval', so the
+ * C library has to translate this into the POSIX compatible layout.
+ */
+ struct __kernel_old_timeval ru_utime;
+ struct __kernel_old_timeval ru_stime;
+#endif
__kernel_long_t ru_maxrss; /* maximum resident set size */
__kernel_long_t ru_ixrss; /* integral shared memory size */
__kernel_long_t ru_idrss; /* integral unshared data size */
diff --git a/kernel/sys.c b/kernel/sys.c
index ad692183dfe9..1de538f622e8 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1769,8 +1769,8 @@ void getrusage(struct task_struct *p, int who, struct rusage *r)
unlock_task_sighand(p, &flags);
out:
- r->ru_utime = ns_to_timeval(utime);
- r->ru_stime = ns_to_timeval(stime);
+ r->ru_utime = ns_to_kernel_old_timeval(utime);
+ r->ru_stime = ns_to_kernel_old_timeval(stime);
if (who != RUSAGE_CHILDREN) {
struct mm_struct *mm = get_task_mm(p);
--
2.9.0
^ permalink raw reply related
* Re: [PATCH v3 01/17] y2038: asm-generic: Extend sysvipc data structures
From: Arnd Bergmann @ 2018-04-20 8:54 UTC (permalink / raw)
To: Eric W. Biederman
Cc: y2038 Mailman List, Linux Kernel Mailing List, Linux API,
linux-arch, GNU C Library, Thomas Gleixner, Deepa Dinamani,
Al Viro, Albert ARIBAUD, linux-s390, Martin Schwidefsky,
the arch/x86 maintainers, Catalin Marinas, Will Deacon,
open list:RALINK MIPS ARCHITECTURE, James Hogan, Ralf Baechle,
linuxppc-dev
In-Reply-To: <87k1t2n8v5.fsf@xmission.com>
On Fri, Apr 20, 2018 at 12:12 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
>
>> On Thu, Apr 19, 2018 at 5:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Apr 19, 2018 at 4:59 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>>> I suspect you want to use __kernel_ulong_t here instead of a raw
>>>> unsigned long. If nothing else it seems inconsistent to use typedefs
>>>> in one half of the structure and no typedefs in the other half.
>>>
>>> Good catch, there is definitely something wrong here, but I think using
>>> __kernel_ulong_t for all members would also be wrong, as that
>>> still changes the layout on x32, which effectively is
>>>
>>> struct msqid64_ds {
>>> ipc64_perm msg_perm;
>>> u64 msg_stime;
>>> u32 __unused1;
>>> /* 32 bit implict padding */
>>> u64 msg_rtime;
>>> u32 __unused2;
>>> /* 32 bit implict padding */
>>> u64 msg_ctime;
>>> u32 __unused3;
>>> /* 32 bit implict padding */
>>> __kernel_pid_t shm_cpid; /* pid of creator */
>>> __kernel_pid_t shm_lpid; /* pid of last operator */
>>> ....
>>> };
>>>
>>> The choices here would be to either use a mix of
>>> __kernel_ulong_t and unsigned long, or taking the x32
>>> version back into arch/x86/include/uapi/asm/ so the
>>> generic version at least makes some sense.
>>>
>>> I can't use __kernel_time_t for the lower half on 32-bit
>>> since it really should be unsigned.
>>
>> After thinking about it some more, I conclude that the structure is simply
>> incorrect on x32: The __kernel_ulong_t usage was introduced in 2013
>> in commit b9cd5ca22d67 ("uapi: Use __kernel_ulong_t in struct
>> msqid64_ds") and apparently was correct initially as __BITS_PER_LONG
>> evaluated to 64, but it broke with commit f4b4aae18288 ("x86/headers/uapi:
>> Fix __BITS_PER_LONG value for x32 builds") that changed the value
>> of __BITS_PER_LONG and introduced the extra padding in 2015.
>>
>> The same change apparently also broke a lot of other definitions, e.g.
>>
>> $ echo "#include <linux/types.h>" | gcc -mx32 -E -xc - | grep -A3
>> __kernel_size_t
>> typedef unsigned int __kernel_size_t;
>> typedef int __kernel_ssize_t;
>> typedef int __kernel_ptrdiff_t;
>>
>> Those used to be defined as 'unsigned long long' and 'long long'
>> respectively, so now all kernel interfaces using those on x32
>> became incompatible!
>
> Is this just for the uapi header as seen by userspace? I expect we are
> using the a normal kernel interface with 64bit longs and 64bit pointers
> when we build the kernel.
Yes, that patch shouldn't have changed anything in the kernel, which
continues to be built with __BITS_PER_LONG=64. I haven't
checked the vdso, which is the only bit of the kernel that gets built
with -mx32, but I assume it's fine as well.
> If this is just a header as seen from userspace mess it seems
> unfortunate but fixable.
Right. I'll fix the IPC stuff for this series to make it work with
any value of __BITS_PER_LONG on x32, but I don't plan to
do anything about the rest of x32. The patch that caused the
problem was intended as a bugfix, so we can't just revert it
without first understanding how to properly fix the original bug,
and which other interfaces have now come to rely on
__BITS_PER_LONG=32 for x32.
Adding a few other folks that have been involved in the x32
kernel support or the Debian port in the past. Maybe one of
them is motivated to figure out how to fix this properly.
Arnd
^ permalink raw reply
* Re: [PATCH v3 04/17] y2038: s390: Remove unneeded ipc uapi header files
From: Arnd Bergmann @ 2018-04-20 7:58 UTC (permalink / raw)
To: Heiko Carstens
Cc: y2038 Mailman List, Linux Kernel Mailing List, Linux API,
linux-arch, GNU C Library, Thomas Gleixner, Deepa Dinamani,
Al Viro, Eric W . Biederman, Albert ARIBAUD, linux-s390,
Martin Schwidefsky, the arch/x86 maintainers, Catalin Marinas,
Will Deacon, open list:RALINK MIPS ARCHITECTURE, James Hogan,
Ralf Baechle
In-Reply-To: <20180420075407.GA7119@osiris>
On Fri, Apr 20, 2018 at 9:54 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Thu, Apr 19, 2018 at 04:37:24PM +0200, Arnd Bergmann wrote:
>> The s390 msgbuf/sembuf/shmbuf header files are all identical to the
>> version from asm-generic.
>>
>> This patch removes the files and replaces them with 'generic-y'
>> statements, to avoid having to modify each copy when we extend sysvipc
>> to deal with 64-bit time_t in 32-bit user space.
>>
>> Note that unlike alpha and ia64, the ipcbuf.h header file is slightly
>> different here, so I'm leaving the private copy.
>>
>> To deal with 32-bit compat tasks, we also have to adapt the definitions
>> of compat_{shm,sem,msg}id_ds to match the changes to the respective
>> asm-generic files.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> arch/s390/include/asm/compat.h | 32 ++++++++++++------------
>> arch/s390/include/uapi/asm/Kbuild | 3 +++
>> arch/s390/include/uapi/asm/msgbuf.h | 38 ----------------------------
>> arch/s390/include/uapi/asm/sembuf.h | 30 -----------------------
>> arch/s390/include/uapi/asm/shmbuf.h | 49 -------------------------------------
>> 5 files changed, 19 insertions(+), 133 deletions(-)
>> delete mode 100644 arch/s390/include/uapi/asm/msgbuf.h
>> delete mode 100644 arch/s390/include/uapi/asm/sembuf.h
>> delete mode 100644 arch/s390/include/uapi/asm/shmbuf.h
>
> FWIW,
>
> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Thanks, added to the patch.
Arnd
^ 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