* Re: [PATCH v3 2/7] shm: add sealing API
From: Hugh Dickins @ 2014-07-16 10:06 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-3-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> If two processes share a common memory region, they usually want some
> guarantees to allow safe access. This often includes:
> - one side cannot overwrite data while the other reads it
> - one side cannot shrink the buffer while the other accesses it
> - one side cannot grow the buffer beyond previously set boundaries
>
> If there is a trust-relationship between both parties, there is no need
> for policy enforcement. However, if there's no trust relationship (eg.,
> for general-purpose IPC) sharing memory-regions is highly fragile and
> often not possible without local copies. Look at the following two
> use-cases:
> 1) A graphics client wants to share its rendering-buffer with a
> graphics-server. The memory-region is allocated by the client for
> read/write access and a second FD is passed to the server. While
> scanning out from the memory region, the server has no guarantee that
> the client doesn't shrink the buffer at any time, requiring rather
> cumbersome SIGBUS handling.
> 2) A process wants to perform an RPC on another process. To avoid huge
> bandwidth consumption, zero-copy is preferred. After a message is
> assembled in-memory and a FD is passed to the remote side, both sides
> want to be sure that neither modifies this shared copy, anymore. The
> source may have put sensible data into the message without a separate
> copy and the target may want to parse the message inline, to avoid a
> local copy.
>
> While SIGBUS handling, POSIX mandatory locking and MAP_DENYWRITE provide
> ways to achieve most of this, the first one is unproportionally ugly to
> use in libraries and the latter two are broken/racy or even disabled due
> to denial of service attacks.
>
> This patch introduces the concept of SEALING. If you seal a file, a
> specific set of operations is blocked on that file forever.
> Unlike locks, seals can only be set, never removed. Hence, once you
> verified a specific set of seals is set, you're guaranteed that no-one can
> perform the blocked operations on this file, anymore.
>
> An initial set of SEALS is introduced by this patch:
> - SHRINK: If SEAL_SHRINK is set, the file in question cannot be reduced
> in size. This affects ftruncate() and open(O_TRUNC).
> - GROW: If SEAL_GROW is set, the file in question cannot be increased
> in size. This affects ftruncate(), fallocate() and write().
> - WRITE: If SEAL_WRITE is set, no write operations (besides resizing)
> are possible. This affects fallocate(PUNCH_HOLE), mmap() and
> write().
> - SEAL: If SEAL_SEAL is set, no further seals can be added to a file.
> This basically prevents the F_ADD_SEAL operation on a file and
> can be set to prevent others from adding further seals that you
> don't want.
>
> The described use-cases can easily use these seals to provide safe use
> without any trust-relationship:
> 1) The graphics server can verify that a passed file-descriptor has
> SEAL_SHRINK set. This allows safe scanout, while the client is
> allowed to increase buffer size for window-resizing on-the-fly.
> Concurrent writes are explicitly allowed.
> 2) For general-purpose IPC, both processes can verify that SEAL_SHRINK,
> SEAL_GROW and SEAL_WRITE are set. This guarantees that neither
> process can modify the data while the other side parses it.
> Furthermore, it guarantees that even with writable FDs passed to the
> peer, it cannot increase the size to hit memory-limits of the source
> process (in case the file-storage is accounted to the source).
>
> The new API is an extension to fcntl(), adding two new commands:
> F_GET_SEALS: Return a bitset describing the seals on the file. This
> can be called on any FD if the underlying file supports
> sealing.
> F_ADD_SEALS: Change the seals of a given file. This requires WRITE
> access to the file and F_SEAL_SEAL may not already be set.
> Furthermore, the underlying file must support sealing and
> there may not be any existing shared mapping of that file.
> Otherwise, EBADF/EPERM is returned.
> The given seals are _added_ to the existing set of seals
> on the file. You cannot remove seals again.
>
> The fcntl() handler is currently specific to shmem and disabled on all
> files. A file needs to explicitly support sealing for this interface to
> work. A separate syscall is added in a follow-up, which creates files that
> support sealing. There is no intention to support this on other
> file-systems. Semantics are unclear for non-volatile files and we lack any
> use-case right now. Therefore, the implementation is specific to shmem.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Looks pretty good to me, minor comments below.
> ---
> fs/fcntl.c | 5 ++
> include/linux/shmem_fs.h | 17 ++++
> include/uapi/linux/fcntl.h | 15 ++++
> mm/shmem.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 223 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 72c82f6..22d1c3d 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -21,6 +21,7 @@
> #include <linux/rcupdate.h>
> #include <linux/pid_namespace.h>
> #include <linux/user_namespace.h>
> +#include <linux/shmem_fs.h>
>
> #include <asm/poll.h>
> #include <asm/siginfo.h>
> @@ -336,6 +337,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> case F_GETPIPE_SZ:
> err = pipe_fcntl(filp, cmd, arg);
> break;
> + case F_ADD_SEALS:
> + case F_GET_SEALS:
> + err = shmem_fcntl(filp, cmd, arg);
> + break;
> default:
> break;
> }
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 4d1771c..50777b5 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -1,6 +1,7 @@
> #ifndef __SHMEM_FS_H
> #define __SHMEM_FS_H
>
> +#include <linux/file.h>
> #include <linux/swap.h>
> #include <linux/mempolicy.h>
> #include <linux/pagemap.h>
> @@ -11,6 +12,7 @@
>
> struct shmem_inode_info {
> spinlock_t lock;
> + unsigned int seals; /* shmem seals */
> unsigned long flags;
> unsigned long alloced; /* data pages alloced to file */
> union {
> @@ -65,4 +67,19 @@ static inline struct page *shmem_read_mapping_page(
> mapping_gfp_mask(mapping));
> }
>
> +#ifdef CONFIG_TMPFS
> +
> +extern int shmem_add_seals(struct file *file, unsigned int seals);
> +extern int shmem_get_seals(struct file *file);
> +extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> +
> +#else
> +
> +static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
> +{
> + return -EINVAL;
> +}
> +
> +#endif
> +
> #endif
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 074b886..beed138 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -28,6 +28,21 @@
> #define F_GETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 8)
>
> /*
> + * Set/Get seals
> + */
> +#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
> +#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
> +
> +/*
> + * Types of seals
> + */
> +#define F_SEAL_SEAL 0x0001 /* prevent further seals from being set */
> +#define F_SEAL_SHRINK 0x0002 /* prevent file from shrinking */
> +#define F_SEAL_GROW 0x0004 /* prevent file from growing */
> +#define F_SEAL_WRITE 0x0008 /* prevent writes */
> +/* (1U << 31) is reserved for signed error codes */
> +
> +/*
> * Types of directory notifications that may be requested.
> */
> #define DN_ACCESS 0x00000001 /* File accessed */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f484c27..1438b3e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -66,6 +66,7 @@ static struct vfsmount *shm_mnt;
> #include <linux/highmem.h>
> #include <linux/seq_file.h>
> #include <linux/magic.h>
> +#include <linux/fcntl.h>
>
> #include <asm/uaccess.h>
> #include <asm/pgtable.h>
> @@ -531,16 +532,23 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
> static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
> {
> struct inode *inode = dentry->d_inode;
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + loff_t oldsize = inode->i_size;
> + loff_t newsize = attr->ia_size;
> int error;
>
> error = inode_change_ok(inode, attr);
> if (error)
> return error;
>
> - if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> - loff_t oldsize = inode->i_size;
> - loff_t newsize = attr->ia_size;
> + /* protected by i_mutex */
> + if (attr->ia_valid & ATTR_SIZE) {
> + if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
> + (newsize > oldsize && (info->seals & F_SEAL_GROW)))
> + return -EPERM;
> + }
Not important but...
I'd have thought all that was better inside the S_ISREG(inode->i_mode) &&
(attr->ia_valid & ATTR_SIZE) block. Less unnecessary change above, and
more efficient for non-size attrs. You cannot seal anything but a regular
file anyway, right?
>
> + if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> if (newsize != oldsize) {
> i_size_write(inode, newsize);
> inode->i_ctime = inode->i_mtime = CURRENT_TIME;
> @@ -1315,6 +1323,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
> info = SHMEM_I(inode);
> memset(info, 0, (char *)inode - (char *)info);
> spin_lock_init(&info->lock);
> + info->seals = F_SEAL_SEAL;
> info->flags = flags & VM_NORESERVE;
> INIT_LIST_HEAD(&info->swaplist);
> simple_xattrs_init(&info->xattrs);
> @@ -1374,7 +1383,15 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
> {
> int ret;
> struct inode *inode = mapping->host;
> + struct shmem_inode_info *info = SHMEM_I(inode);
> pgoff_t index = pos >> PAGE_CACHE_SHIFT;
> +
> + /* i_mutex is held by caller */
> + if (info->seals & F_SEAL_WRITE)
> + return -EPERM;
> + if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
> + return -EPERM;
I think this is your only addition which comes in a hot path.
Mel has been shaving nanoseconds off this path recently: you're not
introducing any atomic ops here, good, but I wonder if it would make any
measurable difference to include this pair of tests inside a single
"if (unlikely(info->seals)) {". Maybe not, but it wouldn't hurt.
> +
> ret = shmem_getpage(inode, index, pagep, SGP_WRITE, NULL);
> if (ret == 0 && *pagep)
> init_page_accessed(*pagep);
> @@ -1715,11 +1732,166 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> return offset;
> }
>
> +/*
> + * Setting SEAL_WRITE requires us to verify there's no pending writer. However,
> + * via get_user_pages(), drivers might have some pending I/O without any active
> + * user-space mappings (eg., direct-IO, AIO). Therefore, we look at all pages
> + * and see whether it has an elevated ref-count. If so, we abort.
> + * The caller must guarantee that no new user will acquire writable references
> + * to those pages to avoid races.
> + */
> +static int shmem_test_for_pins(struct address_space *mapping)
> +{
> + struct radix_tree_iter iter;
> + void **slot;
> + pgoff_t start;
> + struct page *page;
> + int error;
> +
> + /* flush additional refs in lru_add early */
> + lru_add_drain_all();
> +
> + error = 0;
> + start = 0;
> + rcu_read_lock();
> +
> +restart:
> + radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
> + page = radix_tree_deref_slot(slot);
> + if (!page || radix_tree_exception(page)) {
> + if (radix_tree_deref_retry(page))
> + goto restart;
> + } else if (page_count(page) - page_mapcount(page) > 1) {
> + error = -EBUSY;
> + break;
> + }
> +
> + if (need_resched()) {
> + cond_resched_rcu();
> + start = iter.index + 1;
> + goto restart;
> + }
> + }
> + rcu_read_unlock();
> +
> + return error;
> +}
Please leave shmem_test_for_pins() (and the comment above it)
out of this particular patch.
The implementation here satisfies none of us, make it harder to
figure out the patch improving it later, and distracts from the basic
sealing interface and functionality that you introduce in this patch.
A brief comment on the issue instead - "But what if a page of the
object is pinned for pending I/O? See later patch" - maybe, but on the
whole I think it's better to raise and settle the issue in later patch.
> +
> +#define F_ALL_SEALS (F_SEAL_SEAL | \
> + F_SEAL_SHRINK | \
> + F_SEAL_GROW | \
> + F_SEAL_WRITE)
> +
> +int shmem_add_seals(struct file *file, unsigned int seals)
> +{
> + struct dentry *dentry = file->f_path.dentry;
> + struct inode *inode = dentry->d_inode;
struct inode *inode = file_inode(file), and forget about dentry?
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + int error;
> +
> + /*
> + * SEALING
> + * Sealing allows multiple parties to share a shmem-file but restrict
> + * access to a specific subset of file operations. Seals can only be
> + * added, but never removed. This way, mutually untrusted parties can
> + * share common memory regions with a well-defined policy. A malicious
> + * peer can thus never perform unwanted operations on a shared object.
> + *
> + * Seals are only supported on special shmem-files and always affect
> + * the whole underlying inode. Once a seal is set, it may prevent some
> + * kinds of access to the file. Currently, the following seals are
> + * defined:
> + * SEAL_SEAL: Prevent further seals from being set on this file
> + * SEAL_SHRINK: Prevent the file from shrinking
> + * SEAL_GROW: Prevent the file from growing
> + * SEAL_WRITE: Prevent write access to the file
> + *
> + * As we don't require any trust relationship between two parties, we
> + * must prevent seals from being removed. Therefore, sealing a file
> + * only adds a given set of seals to the file, it never touches
> + * existing seals. Furthermore, the "setting seals"-operation can be
> + * sealed itself, which basically prevents any further seal from being
> + * added.
> + *
> + * Semantics of sealing are only defined on volatile files. Only
> + * anonymous shmem files support sealing. More importantly, seals are
> + * never written to disk. Therefore, there's no plan to support it on
> + * other file types.
> + */
> +
> + if (file->f_op != &shmem_file_operations)
> + return -EINVAL;
> + if (!(file->f_mode & FMODE_WRITE))
> + return -EINVAL;
I would expect -EBADF there (like when you write to read-only fd).
Though I was okay with the -EPERM you had the previous version.
> + if (seals & ~(unsigned int)F_ALL_SEALS)
> + return -EINVAL;
> +
> + mutex_lock(&inode->i_mutex);
> +
> + if (info->seals & F_SEAL_SEAL) {
I notice this is inconsistent with F_SEAL_WRITE just below:
we're allowed to SEAL_WRITE what's already SEAL_WRITEd,
but not to SEAL_SEAL what's already SEAL_SEALed.
Oh, never mind, I can see that makes some sense.
> + error = -EPERM;
> + goto unlock;
> + }
> +
> + if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
> + error = mapping_deny_writable(file->f_mapping);
> + if (error)
Which would be -EBUSY, yes, that seems okay.
And with your atomic i_mmap_writable changes in 1/7, and the i_mutex
here, the locking is now solid, and accomplished simply: nice.
> + goto unlock;
> +
> + error = shmem_test_for_pins(file->f_mapping);
> + if (error) {
> + mapping_allow_writable(file->f_mapping);
> + goto unlock;
> + }
Right, although I ask you to remove shmem_test_for_pins() from this
patch, I can see that you might want to include a "return 0" stub for
shmem_wait_for_pins() in this patch, just so that this can appear here
now, and we consider the non-atomicity of it. Yes, I agree this is
how it should proceed: first deny, then re-allow if waiting fails.
> + }
> +
> + info->seals |= seals;
> + error = 0;
> +
> +unlock:
> + mutex_unlock(&inode->i_mutex);
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(shmem_add_seals);
> +
> +int shmem_get_seals(struct file *file)
> +{
> + if (file->f_op != &shmem_file_operations)
> + return -EINVAL;
That's fine, though it is worth considering whether return 0
might be preferable. No, I suppose this is easier, fits with
shmem_fcntl() just returning -EINVAL when !TMPFS or !SHMEM.
> +
> + return SHMEM_I(file_inode(file))->seals & F_ALL_SEALS;
& F_ALL_SEALS? Okay, that may be some kind of future proofing that you
have in mind; but it may just be a leftover from when you were using bit
31 for internal use.
> +}
> +EXPORT_SYMBOL_GPL(shmem_get_seals);
> +
> +long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + long error;
> +
> + switch (cmd) {
> + case F_ADD_SEALS:
> + /* disallow upper 32bit */
> + if (arg >> 32)
> + return -EINVAL;
That is worth checking, but gives
mm/shmem.c:1948:3: warning: right shift count >= width of type
on a 32-bit build. I expect there's an accepted way to do it;
I've used "arg > UINT_MAX" myself in some places.
> +
> + error = shmem_add_seals(file, arg);
> + break;
> + case F_GET_SEALS:
> + error = shmem_get_seals(file);
> + break;
> + default:
> + error = -EINVAL;
> + break;
> + }
> +
> + return error;
> +}
> +
> static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> loff_t len)
> {
> struct inode *inode = file_inode(file);
> struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
> + struct shmem_inode_info *info = SHMEM_I(inode);
> struct shmem_falloc shmem_falloc;
> pgoff_t start, index, end;
> int error;
> @@ -1731,6 +1903,12 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> loff_t unmap_start = round_up(offset, PAGE_SIZE);
> loff_t unmap_end = round_down(offset + len, PAGE_SIZE) - 1;
>
> + /* protected by i_mutex */
> + if (info->seals & F_SEAL_WRITE) {
> + error = -EPERM;
> + goto out;
> + }
> +
> if ((u64)unmap_end > (u64)unmap_start)
> unmap_mapping_range(mapping, unmap_start,
> 1 + unmap_end - unmap_start, 0);
> @@ -1745,6 +1923,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> if (error)
> goto out;
>
> + if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
> + error = -EPERM;
> + goto out;
> + }
> +
> start = offset >> PAGE_CACHE_SHIFT;
> end = (offset + len + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> /* Try to avoid a swapstorm if len is impossible to satisfy */
> --
> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [patch 4/4] timerfd.2: Add ioctl method description
From: Cyrill Gorcunov @ 2014-07-15 21:54 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: Michael Kerrisk, Thomas Gleixner, Cyrill Gorcunov, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715215451.499191426@openvz.org>
[-- Attachment #1: timerfd-man --]
[-- Type: text/plain, Size: 1806 bytes --]
ioctl(2)
The following commands are supported: TFD_IOC_SET_TICKS to adjust
the number of the timer expirations that have occurred.
It take a pointer to nonzero 8-byte integer (uint64_t*) containing
new number of expirations. Once the number is set any waiter on
the timer is woken up. The only purpose of this command is to restore
the expirations in a sake of checkpoint/restore procedure.
It requires the kernel to be built with CONFIG_CHECKPOINT_RESTORE
support.
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
CC: linux-api@vger.kernel.org
---
man2/timerfd_create.2 | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Index: man-pages/man2/timerfd_create.2
===================================================================
--- man-pages.orig/man2/timerfd_create.2
+++ man-pages/man2/timerfd_create.2
@@ -260,6 +260,20 @@ multiplexing APIs:
and
.BR epoll (7).
.TP
+.BR ioctl "(2)"
+The following commands are supported:
+.B TFD_IOC_SET_TICKS
+to adjust the number of the timer expirations that have occurred.
+It take a pointer to nonzero 8-byte integer
+.RI ( uint64_t *)
+containing the new number of expirations.
+Once the number is set any waiter on the timer is woken up.
+The only purpose of this command is to restore the expirations
+in a sake of checkpoint/restore procedure.
+It requires the kernel to be built with
+.BR CONFIG_CHECKPOINT_RESTORE
+support.
+.TP
.BR close (2)
When the file descriptor is no longer required it should be closed.
When all file descriptors associated with the same timer object
^ permalink raw reply
* [patch 3/4] timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks, v3
From: Cyrill Gorcunov @ 2014-07-15 21:54 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Michael Kerrisk, Thomas Gleixner, Cyrill Gorcunov, Andrew Morton,
Andrey Vagin, Arnd Bergmann, Christopher Covington,
Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715215451.499191426@openvz.org>
[-- Attachment #1: timerfd-ticks-ioctl-5 --]
[-- Type: text/plain, Size: 3377 bytes --]
The read() of timerfd files allows to fetch the number of timer ticks
while there is no way to set it back from userspace.
To restore the timer's state as it was at checkpoint moment we need
a path to bring @ticks back. Initially I thought about writing ticks
back via write() interface but it seems such API is somehow obscure.
Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
command which allows to adjust @ticks into non-zero value waking
up the waiters.
I wrapped code with CONFIG_CHECKPOINT_RESTORE which can be
dropped off if there users except c/r camp appear.
v2 (by akpm@):
- Use define timerfd_ioctl NULL for non c/r config
v3:
- Use copy_from_user for @ticks fetching since
not all arch support get_user for 8 byte argument
CC: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
CC: Christopher Covington <cov-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---
fs/timerfd.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/timerfd.h | 5 +++++
2 files changed, 42 insertions(+)
Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -315,12 +315,49 @@ static int timerfd_show(struct seq_file
#define timerfd_show NULL
#endif
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct timerfd_ctx *ctx = file->private_data;
+ int ret = 0;
+
+ switch (cmd) {
+ case TFD_IOC_SET_TICKS: {
+ u64 ticks;
+
+ if (copy_from_user(&ticks, (u64 __user *)arg, sizeof(ticks)))
+ return -EFAULT;
+ if (!ticks)
+ return -EINVAL;
+
+ spin_lock_irq(&ctx->wqh.lock);
+ if (!timerfd_canceled(ctx)) {
+ ctx->ticks = ticks;
+ if (ticks)
+ wake_up_locked(&ctx->wqh);
+ } else
+ ret = -ECANCELED;
+ spin_unlock_irq(&ctx->wqh.lock);
+ break;
+ }
+ default:
+ ret = -ENOTTY;
+ break;
+ }
+
+ return ret;
+}
+#else
+#define timerfd_ioctl NULL
+#endif
+
static const struct file_operations timerfd_fops = {
.release = timerfd_release,
.poll = timerfd_poll,
.read = timerfd_read,
.llseek = noop_llseek,
.show_fdinfo = timerfd_show,
+ .unlocked_ioctl = timerfd_ioctl,
};
static int timerfd_fget(int fd, struct fd *p)
Index: linux-2.6.git/include/linux/timerfd.h
===================================================================
--- linux-2.6.git.orig/include/linux/timerfd.h
+++ linux-2.6.git/include/linux/timerfd.h
@@ -11,6 +11,9 @@
/* For O_CLOEXEC and O_NONBLOCK */
#include <linux/fcntl.h>
+/* For _IO helpers */
+#include <linux/ioctl.h>
+
/*
* CAREFUL: Check include/asm-generic/fcntl.h when defining
* new flags, since they might collide with O_* ones. We want
@@ -29,4 +32,6 @@
/* Flags for timerfd_settime. */
#define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
+#define TFD_IOC_SET_TICKS _IOW('T', 0, u64)
+
#endif /* _LINUX_TIMERFD_H */
^ permalink raw reply
* [patch 2/4] docs: procfs -- Document timerfd output
From: Cyrill Gorcunov @ 2014-07-15 21:54 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Michael Kerrisk, Thomas Gleixner, Cyrill Gorcunov, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715215451.499191426@openvz.org>
[-- Attachment #1: timerfd-doc-fdinfo --]
[-- Type: text/plain, Size: 1741 bytes --]
CC: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---
Documentation/filesystems/proc.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -1743,6 +1743,25 @@ pair provide additional information part
While the first three lines are mandatory and always printed, the rest is
optional and may be omitted if no marks created yet.
+ Timerfd files
+ ~~~~~~~~~~~~~
+
+ pos: 0
+ flags: 02
+ mnt_id: 9
+ clockid: 0
+ ticks: 0
+ settime flags: 01
+ it_value: (0, 49406829)
+ it_interval: (1, 0)
+
+ where 'clockid' is the clock type and 'ticks' is the number of the timer expirations
+ that have occurred [see timerfd_create(2) for details]. 'settime flags' are
+ flags in octal form been used to setup the timer [see timerfd_settime(2) for
+ details]. 'it_value' is remaining time until the timer exiration.
+ 'it_interval' is the interval for the timer. Note the timer might be set up
+ with TIMER_ABSTIME option which will be shown in 'settime flags', but 'it_value'
+ still exhibits timer's remaining time.
------------------------------------------------------------------------------
Configuring procfs
^ permalink raw reply
* [patch 1/4] timerfd: Implement show_fdinfo method
From: Cyrill Gorcunov @ 2014-07-15 21:54 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Michael Kerrisk, Thomas Gleixner, Cyrill Gorcunov, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715215451.499191426@openvz.org>
[-- Attachment #1: timerfd-show-fdinfo-5 --]
[-- Type: text/plain, Size: 3280 bytes --]
For checkpoint/restore of timerfd files we need to know how exactly
the timer were armed, to be able to recreate it on restore stage.
Thus implement show_fdinfo method which provides enough information
for that.
One of significant changes I think is the addition of @settime_flags
member. Currently there are two flags TFD_TIMER_ABSTIME and
TFD_TIMER_CANCEL_ON_SET, and the second can be found from
@might_cancel variable but in case if the flags will be extended
in future we most probably will have to somehow remember them
explicitly anyway so I guss doing that right now won't hurt.
To not bloat the timerfd_ctx structure I've converted @expired
to short integer and defined @settime_flags as short too.
v2 (by avagin@, vdavydov@ and tglx@):
- Add it_value/it_interval fields
- Save flags being used in timerfd_setup in context
v3 (by tglx@):
- don't forget to use CONFIG_PROC_FS
v4 (by akpm@):
-Use define timerfd_show NULL for non c/r config
CC: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---
fs/timerfd.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -35,8 +35,9 @@ struct timerfd_ctx {
ktime_t moffs;
wait_queue_head_t wqh;
u64 ticks;
- int expired;
int clockid;
+ short unsigned expired;
+ short unsigned settime_flags; /* to show in fdinfo */
struct rcu_head rcu;
struct list_head clist;
bool might_cancel;
@@ -196,6 +197,8 @@ static int timerfd_setup(struct timerfd_
if (timerfd_canceled(ctx))
return -ECANCELED;
}
+
+ ctx->settime_flags = flags & TFD_SETTIME_FLAGS;
return 0;
}
@@ -284,11 +287,40 @@ static ssize_t timerfd_read(struct file
return res;
}
+#ifdef CONFIG_PROC_FS
+static int timerfd_show(struct seq_file *m, struct file *file)
+{
+ struct timerfd_ctx *ctx = file->private_data;
+ struct itimerspec t;
+
+ spin_lock_irq(&ctx->wqh.lock);
+ t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
+ t.it_interval = ktime_to_timespec(ctx->tintv);
+ spin_unlock_irq(&ctx->wqh.lock);
+
+ return seq_printf(m,
+ "clockid: %d\n"
+ "ticks: %llu\n"
+ "settime flags: 0%o\n"
+ "it_value: (%llu, %llu)\n"
+ "it_interval: (%llu, %llu)\n",
+ ctx->clockid, (unsigned long long)ctx->ticks,
+ ctx->settime_flags,
+ (unsigned long long)t.it_value.tv_sec,
+ (unsigned long long)t.it_value.tv_nsec,
+ (unsigned long long)t.it_interval.tv_sec,
+ (unsigned long long)t.it_interval.tv_nsec);
+}
+#else
+#define timerfd_show NULL
+#endif
+
static const struct file_operations timerfd_fops = {
.release = timerfd_release,
.poll = timerfd_poll,
.read = timerfd_read,
.llseek = noop_llseek,
+ .show_fdinfo = timerfd_show,
};
static int timerfd_fget(int fd, struct fd *p)
^ permalink raw reply
* [patch 0/4] timerfd c/r support, v5
From: Cyrill Gorcunov @ 2014-07-15 21:54 UTC (permalink / raw)
To: linux-kernel, linux-api; +Cc: Michael Kerrisk, Thomas Gleixner, Cyrill Gorcunov
Hi, sorry for spamming, but in v4 I simply messed up in the series,
doing reply-to-reply with updated variants. So here are all patches
up to date on top of -rc5. Sorry again for resend.
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Thomas Gleixner @ 2014-07-15 21:20 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715211800.GM23835@moon>
On Wed, 16 Jul 2014, Cyrill Gorcunov wrote:
> On Tue, Jul 15, 2014 at 11:16:23PM +0200, Thomas Gleixner wrote:
> > >
> > > Maybe it's because of new -rc? Letme check...
> >
> > No, it's because its against an older version of your patches which
> > lack the #ifdef PROC_FS around the show function ....
>
> Crap. Sorry. Thomas, I'm fetching -rc5 now, I suppose better will
> be if I rebase the whole series and resend. Sounds good?
No rebase necessary. 1&2 apply fine. It's home made wreckage :)
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Cyrill Gorcunov @ 2014-07-15 21:18 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <alpine.DEB.2.10.1407152315350.24854@nanos>
On Tue, Jul 15, 2014 at 11:16:23PM +0200, Thomas Gleixner wrote:
> >
> > Maybe it's because of new -rc? Letme check...
>
> No, it's because its against an older version of your patches which
> lack the #ifdef PROC_FS around the show function ....
Crap. Sorry. Thomas, I'm fetching -rc5 now, I suppose better will
be if I rebase the whole series and resend. Sounds good?
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Thomas Gleixner @ 2014-07-15 21:16 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715211413.GL23835@moon>
On Wed, 16 Jul 2014, Cyrill Gorcunov wrote:
> On Tue, Jul 15, 2014 at 11:08:39PM +0200, Thomas Gleixner wrote:
> > On Tue, 15 Jul 2014, Cyrill Gorcunov wrote:
> >
> > > On Tue, Jul 15, 2014 at 06:11:58PM +0200, Thomas Gleixner wrote:
> > > > >
> > > > > Dear sirs, ping?
> > > >
> > > > Hmm, I was waiting for a V5, but it seems you just updated that single
> > > > patch according to the review. I'll pick it up
> > >
> > > Thanks a lot, Thomas!
> >
> > Sigh. That updated 3/4 patch does not even apply....
>
> Maybe it's because of new -rc? Letme check...
No, it's because its against an older version of your patches which
lack the #ifdef PROC_FS around the show function ....
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Cyrill Gorcunov @ 2014-07-15 21:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <alpine.DEB.2.10.1407152306430.24854@nanos>
On Tue, Jul 15, 2014 at 11:08:39PM +0200, Thomas Gleixner wrote:
> On Tue, 15 Jul 2014, Cyrill Gorcunov wrote:
>
> > On Tue, Jul 15, 2014 at 06:11:58PM +0200, Thomas Gleixner wrote:
> > > >
> > > > Dear sirs, ping?
> > >
> > > Hmm, I was waiting for a V5, but it seems you just updated that single
> > > patch according to the review. I'll pick it up
> >
> > Thanks a lot, Thomas!
>
> Sigh. That updated 3/4 patch does not even apply....
Maybe it's because of new -rc? Letme check...
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Thomas Gleixner @ 2014-07-15 21:08 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715162501.GK23835@moon>
On Tue, 15 Jul 2014, Cyrill Gorcunov wrote:
> On Tue, Jul 15, 2014 at 06:11:58PM +0200, Thomas Gleixner wrote:
> > >
> > > Dear sirs, ping?
> >
> > Hmm, I was waiting for a V5, but it seems you just updated that single
> > patch according to the review. I'll pick it up
>
> Thanks a lot, Thomas!
Sigh. That updated 3/4 patch does not even apply....
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Cyrill Gorcunov @ 2014-07-15 16:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <alpine.DEB.2.10.1407151811050.24854@nanos>
On Tue, Jul 15, 2014 at 06:11:58PM +0200, Thomas Gleixner wrote:
> >
> > Dear sirs, ping?
>
> Hmm, I was waiting for a V5, but it seems you just updated that single
> patch according to the review. I'll pick it up
Thanks a lot, Thomas!
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Thomas Gleixner @ 2014-07-15 16:11 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk, Andrew Morton,
Andrey Vagin, Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140715135445.GJ23835@moon>
On Tue, 15 Jul 2014, Cyrill Gorcunov wrote:
> On Mon, Jun 30, 2014 at 11:43:54PM +0400, Cyrill Gorcunov wrote:
> > On Mon, Jun 23, 2014 at 10:54:31PM +0400, Cyrill Gorcunov wrote:
> > >
> > > Hi guys, here is an updated version of c/r support for timerfd files. The main change
> > > is in how @ticks are restored in patch 3 -- I switched to ioctl code, which is wrapped
> > > with CONFIG because I still think that while there is only one ioctl designated
> > > solely for c/r needs no need to build it all the time until explicitly requested.
> > > Please take a look once time permit. Comments are highly appreciated.
> > > Also note the last patch is for man-page git repo, not for kernel.
> >
> > Gentlemen, could you please point me if there something preventing the
> > series from being picked up? Or there some way to improve the series?
>
> Dear sirs, ping?
Hmm, I was waiting for a V5, but it seems you just updated that single
patch according to the review. I'll pick it up
^ permalink raw reply
* Re: [patch 0/4] timerfd c/r support, v4
From: Cyrill Gorcunov @ 2014-07-15 13:54 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: Thomas Gleixner, Michael Kerrisk, Andrew Morton, Andrey Vagin,
Pavel Emelyanov, Vladimir Davydov
In-Reply-To: <20140630194354.GB2305@moon>
On Mon, Jun 30, 2014 at 11:43:54PM +0400, Cyrill Gorcunov wrote:
> On Mon, Jun 23, 2014 at 10:54:31PM +0400, Cyrill Gorcunov wrote:
> >
> > Hi guys, here is an updated version of c/r support for timerfd files. The main change
> > is in how @ticks are restored in patch 3 -- I switched to ioctl code, which is wrapped
> > with CONFIG because I still think that while there is only one ioctl designated
> > solely for c/r needs no need to build it all the time until explicitly requested.
> > Please take a look once time permit. Comments are highly appreciated.
> > Also note the last patch is for man-page git repo, not for kernel.
>
> Gentlemen, could you please point me if there something preventing the
> series from being picked up? Or there some way to improve the series?
Dear sirs, ping?
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: James Morris @ 2014-07-15 1:53 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Andy Lutomirski, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <CAGXu5jJJsiTxxn5UijkBz7jpWgqg01BS=Zc0WbHXbs0vH_xPMQ@mail.gmail.com>
On 07/15/2014 04:59 AM, Kees Cook wrote:
> Hi James,
>
> Is this series something you would carry in the security-next tree?
> That has traditionally been where seccomp features have landed in the
> past.
>
> -Kees
>
>
> On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 07/10, Kees Cook wrote:
>>>>
>>>> This adds the ability for threads to request seccomp filter
>>>> synchronization across their thread group (at filter attach time).
>>>> For example, for Chrome to make sure graphic driver threads are fully
>>>> confined after seccomp filters have been attached.
>>>>
>>>> To support this, locking on seccomp changes via thread-group-shared
>>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>>> with thread creation are handled via delayed duplication of the seccomp
>>>> task struct field and cred_guard_mutex.
>>>>
>>>> This includes a new syscall (instead of adding a new prctl option),
>>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>>
>>> I do not not see any problems in this version,
>>
>> Awesome! Thank you for all the reviews. :) If Andy and Michael are
>> happy with this too, I think this is in good shape. \o/
>>
>> -Kees
>>
>>>
>>> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>>>
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>
>
>
Yep, certainly.
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-14 20:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <CALCETrVXgA9a2f7VwnCYW4_XB+JAPRSR8xsuH_ZYbA82=ZozRw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Jul 14, 2014 at 12:04 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On 07/10, Kees Cook wrote:
>>>>
>>>> This adds the ability for threads to request seccomp filter
>>>> synchronization across their thread group (at filter attach time).
>>>> For example, for Chrome to make sure graphic driver threads are fully
>>>> confined after seccomp filters have been attached.
>>>>
>>>> To support this, locking on seccomp changes via thread-group-shared
>>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>>> with thread creation are handled via delayed duplication of the seccomp
>>>> task struct field and cred_guard_mutex.
>>>>
>>>> This includes a new syscall (instead of adding a new prctl option),
>>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>>
>>> I do not not see any problems in this version,
>>
>> Awesome! Thank you for all the reviews. :) If Andy and Michael are
>> happy with this too, I think this is in good shape. \o/
>
> I think I'm happy with it. Is it in git somewhere for easy perusal?
> I have a cold, so my reviewing ability is a bit off, but I want to
> take a look at the final version, and git is a little easier than
> email for this.
Yes, absolutely:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=seccomp-tsync
Thanks for looking it over, and I hope you feel better soon!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Andy Lutomirski @ 2014-07-14 19:04 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jK-x0=Rr7kX2a=b4Z8ueA77uwmhNZZAayG8cwmNOKa8Ug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 07/10, Kees Cook wrote:
>>>
>>> This adds the ability for threads to request seccomp filter
>>> synchronization across their thread group (at filter attach time).
>>> For example, for Chrome to make sure graphic driver threads are fully
>>> confined after seccomp filters have been attached.
>>>
>>> To support this, locking on seccomp changes via thread-group-shared
>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>> with thread creation are handled via delayed duplication of the seccomp
>>> task struct field and cred_guard_mutex.
>>>
>>> This includes a new syscall (instead of adding a new prctl option),
>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>
>> I do not not see any problems in this version,
>
> Awesome! Thank you for all the reviews. :) If Andy and Michael are
> happy with this too, I think this is in good shape. \o/
I think I'm happy with it. Is it in git somewhere for easy perusal?
I have a cold, so my reviewing ability is a bit off, but I want to
take a look at the final version, and git is a little easier than
email for this.
--Andy
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-14 18:59 UTC (permalink / raw)
To: James Morris
Cc: Oleg Nesterov, LKML, Andy Lutomirski, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jK-x0=Rr7kX2a=b4Z8ueA77uwmhNZZAayG8cwmNOKa8Ug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi James,
Is this series something you would carry in the security-next tree?
That has traditionally been where seccomp features have landed in the
past.
-Kees
On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 07/10, Kees Cook wrote:
>>>
>>> This adds the ability for threads to request seccomp filter
>>> synchronization across their thread group (at filter attach time).
>>> For example, for Chrome to make sure graphic driver threads are fully
>>> confined after seccomp filters have been attached.
>>>
>>> To support this, locking on seccomp changes via thread-group-shared
>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>> with thread creation are handled via delayed duplication of the seccomp
>>> task struct field and cred_guard_mutex.
>>>
>>> This includes a new syscall (instead of adding a new prctl option),
>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>
>> I do not not see any problems in this version,
>
> Awesome! Thank you for all the reviews. :) If Andy and Michael are
> happy with this too, I think this is in good shape. \o/
>
> -Kees
>
>>
>> Reviewed-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>
>
>
> --
> Kees Cook
> Chrome OS Security
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
From: Gabbay, Oded @ 2014-07-14 7:33 UTC (permalink / raw)
To: airlied@gmail.com
Cc: oded.gabbay@gmail.com, Lewycky, Andrew, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Pinchuk, Evgeny, Deucher, Alexander, Skidanov, Alexey
In-Reply-To: <CAPM=9tyV8PBreHHSsMKo8dRiGvVWHR+LLPcNE2dVpoYkftpv2Q@mail.gmail.com>
On Sat, 2014-07-12 at 07:42 +1000, Dave Airlie wrote:
> > +/* The 64-bit ABI is the authoritative version. */
> > +#pragma pack(push, 8)
> > +
>
> Don't do this, pad and align things explicitly in structs.
>
> > +struct kfd_ioctl_create_queue_args {
> > + uint64_t ring_base_address; /* to KFD */
> > + uint32_t ring_size; /* to KFD */
> > + uint32_t gpu_id; /* to KFD */
> > + uint32_t queue_type; /* to KFD */
> > + uint32_t queue_percentage; /* to KFD */
> > + uint32_t queue_priority; /* to KFD */
> > + uint64_t write_pointer_address; /* to KFD */
> > + uint64_t read_pointer_address; /* to KFD */
> > +
> > + uint64_t doorbell_address; /* from KFD */
> > + uint32_t queue_id; /* from KFD */
> > +};
> > +
>
> maybe put all the uint64_t at the start, or add explicit padding.
>
> Dave.
Thanks, will be fixed.
Oded
^ permalink raw reply
* Re: [PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
From: Dave Airlie @ 2014-07-11 21:42 UTC (permalink / raw)
To: Oded Gabbay
Cc: Andrew Lewycky, Ben Goz, LKML, dri-devel, Evgeny Pinchuk,
Alexey Skidanov, linux-api, Alex Deucher
In-Reply-To: <1405029027-6085-12-git-send-email-oded.gabbay@amd.com>
> +/* The 64-bit ABI is the authoritative version. */
> +#pragma pack(push, 8)
> +
Don't do this, pad and align things explicitly in structs.
> +struct kfd_ioctl_create_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> + uint32_t ring_size; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> + uint32_t queue_type; /* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority; /* to KFD */
> + uint64_t write_pointer_address; /* to KFD */
> + uint64_t read_pointer_address; /* to KFD */
> +
> + uint64_t doorbell_address; /* from KFD */
> + uint32_t queue_id; /* from KFD */
> +};
> +
maybe put all the uint64_t at the start, or add explicit padding.
Dave.
^ permalink raw reply
* Re: [PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
From: Jerome Glisse @ 2014-07-11 21:01 UTC (permalink / raw)
To: Oded Gabbay
Cc: Andrew Lewycky, Ben Goz, linux-kernel, dri-devel, Evgeny Pinchuk,
Alexey Skidanov, linux-api, Alex Deucher
In-Reply-To: <1405029027-6085-12-git-send-email-oded.gabbay@amd.com>
On Fri, Jul 11, 2014 at 12:50:13AM +0300, Oded Gabbay wrote:
> This patch adds 2 new IOCTL to kfd driver.
>
> The first IOCTL is KFD_IOC_CREATE_QUEUE that is used by the user-mode
> application to create a compute queue on the GPU.
>
> The second IOCTL is KFD_IOC_DESTROY_QUEUE that is used by the
> user-mode application to destroy an existing compute queue on the GPU.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
> ---
> drivers/gpu/hsa/radeon/kfd_chardev.c | 155 ++++++++++++++++++++++++++++++++++
> drivers/gpu/hsa/radeon/kfd_doorbell.c | 11 +++
> include/uapi/linux/kfd_ioctl.h | 69 +++++++++++++++
> 3 files changed, 235 insertions(+)
> create mode 100644 include/uapi/linux/kfd_ioctl.h
>
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index 0b5bc74..4e7d5d0 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -27,11 +27,13 @@
> #include <linux/sched.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <uapi/linux/kfd_ioctl.h>
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
>
> static long kfd_ioctl(struct file *, unsigned int, unsigned long);
> static int kfd_open(struct inode *, struct file *);
> +static int kfd_mmap(struct file *, struct vm_area_struct *);
>
> static const char kfd_dev_name[] = "kfd";
>
> @@ -108,17 +110,170 @@ kfd_open(struct inode *inode, struct file *filep)
> return 0;
> }
>
> +static long
> +kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_create_queue_args args;
> + struct kfd_dev *dev;
> + int err = 0;
> + unsigned int queue_id;
> + struct kfd_queue *queue;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + dev = radeon_kfd_device_by_id(args.gpu_id);
> + if (dev == NULL)
> + return -EINVAL;
> +
> + queue = kzalloc(
> + offsetof(struct kfd_queue, scheduler_queue) + dev->device_info->scheduler_class->queue_size,
> + GFP_KERNEL);
> +
> + if (!queue)
> + return -ENOMEM;
> +
> + queue->dev = dev;
> +
> + mutex_lock(&p->mutex);
> +
> + pdd = radeon_kfd_bind_process_to_device(dev, p);
> + if (IS_ERR(pdd) < 0) {
> + err = PTR_ERR(pdd);
> + goto err_bind_pasid;
> + }
> +
> + pr_debug("kfd: creating queue number %d for PASID %d on GPU 0x%x\n",
> + pdd->queue_count,
> + p->pasid,
> + dev->id);
> +
> + if (pdd->queue_count++ == 0) {
> + err = dev->device_info->scheduler_class->register_process(dev->scheduler, p, &pdd->scheduler_process);
> + if (err < 0)
> + goto err_register_process;
> + }
> +
> + if (!radeon_kfd_allocate_queue_id(p, &queue_id))
> + goto err_allocate_queue_id;
> +
> + err = dev->device_info->scheduler_class->create_queue(dev->scheduler, pdd->scheduler_process,
> + &queue->scheduler_queue,
> + (void __user *)args.ring_base_address,
> + args.ring_size,
> + (void __user *)args.read_pointer_address,
> + (void __user *)args.write_pointer_address,
> + radeon_kfd_queue_id_to_doorbell(dev, p, queue_id));
> + if (err)
> + goto err_create_queue;
> +
> + radeon_kfd_install_queue(p, queue_id, queue);
> +
> + args.queue_id = queue_id;
> + args.doorbell_address = (uint64_t)(uintptr_t)radeon_kfd_get_doorbell(filep, p, dev, queue_id);
> +
> + if (copy_to_user(arg, &args, sizeof(args))) {
> + err = -EFAULT;
> + goto err_copy_args_out;
> + }
> +
> + mutex_unlock(&p->mutex);
> +
> + pr_debug("kfd: queue id %d was created successfully.\n"
> + " ring buffer address == 0x%016llX\n"
> + " read ptr address == 0x%016llX\n"
> + " write ptr address == 0x%016llX\n"
> + " doorbell address == 0x%016llX\n",
> + args.queue_id,
> + args.ring_base_address,
> + args.read_pointer_address,
> + args.write_pointer_address,
> + args.doorbell_address);
> +
> + return 0;
> +
> +err_copy_args_out:
> + dev->device_info->scheduler_class->destroy_queue(dev->scheduler, &queue->scheduler_queue);
> +err_create_queue:
> + radeon_kfd_remove_queue(p, queue_id);
> +err_allocate_queue_id:
> + if (--pdd->queue_count == 0) {
> + dev->device_info->scheduler_class->deregister_process(dev->scheduler, pdd->scheduler_process);
> + pdd->scheduler_process = NULL;
> + }
> +err_register_process:
> +err_bind_pasid:
> + kfree(queue);
> + mutex_unlock(&p->mutex);
> + return err;
> +}
> +
> +static int
> +kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_destroy_queue_args args;
> + struct kfd_queue *queue;
> + struct kfd_dev *dev;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + mutex_lock(&p->mutex);
> +
> + queue = radeon_kfd_get_queue(p, args.queue_id);
> + if (!queue) {
> + mutex_unlock(&p->mutex);
> + return -EINVAL;
> + }
> +
> + dev = queue->dev;
> +
> + pr_debug("kfd: destroying queue id %d for PASID %d\n",
> + args.queue_id,
> + p->pasid);
> +
> + radeon_kfd_remove_queue(p, args.queue_id);
> + dev->device_info->scheduler_class->destroy_queue(dev->scheduler, &queue->scheduler_queue);
> +
> + kfree(queue);
> +
> + pdd = radeon_kfd_get_process_device_data(dev, p);
> + BUG_ON(pdd == NULL); /* Because a queue exists. */
> +
> + if (--pdd->queue_count == 0) {
> + dev->device_info->scheduler_class->deregister_process(dev->scheduler, pdd->scheduler_process);
> + pdd->scheduler_process = NULL;
> + }
> +
> + mutex_unlock(&p->mutex);
> + return 0;
> +}
>
> static long
> kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> {
> + struct kfd_process *process;
> long err = -EINVAL;
>
> dev_info(kfd_device,
> "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
> cmd, _IOC_NR(cmd), arg);
>
> + process = radeon_kfd_get_process(current);
> + if (IS_ERR(process))
> + return PTR_ERR(process);
> +
> switch (cmd) {
> + case KFD_IOC_CREATE_QUEUE:
> + err = kfd_ioctl_create_queue(filep, process, (void __user *)arg);
> + break;
> +
> + case KFD_IOC_DESTROY_QUEUE:
> + err = kfd_ioctl_destroy_queue(filep, process, (void __user *)arg);
> + break;
> +
> default:
> dev_err(kfd_device,
> "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
> diff --git a/drivers/gpu/hsa/radeon/kfd_doorbell.c b/drivers/gpu/hsa/radeon/kfd_doorbell.c
> index e1d8506..3de8a02 100644
> --- a/drivers/gpu/hsa/radeon/kfd_doorbell.c
> +++ b/drivers/gpu/hsa/radeon/kfd_doorbell.c
> @@ -155,3 +155,14 @@ doorbell_t __user *radeon_kfd_get_doorbell(struct file *devkfd, struct kfd_proce
> return &pdd->doorbell_mapping[doorbell_index];
> }
>
> +/*
> + * queue_ids are in the range [0,MAX_PROCESS_QUEUES) and are mapped 1:1
> + * to doorbells with the process's doorbell page
> + */
> +unsigned int radeon_kfd_queue_id_to_doorbell(struct kfd_dev *kfd, struct kfd_process *process, unsigned int queue_id)
> +{
> + /* doorbell_id_offset accounts for doorbells taken by KGD.
> + * pasid * doorbell_process_allocation/sizeof(doorbell_t) adjusts to the process's doorbells */
> + return kfd->doorbell_id_offset + process->pasid * (doorbell_process_allocation()/sizeof(doorbell_t)) + queue_id;
> +}
> +
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> new file mode 100644
> index 0000000..dcc5fe0
> --- /dev/null
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright 2014 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef KFD_IOCTL_H_INCLUDED
> +#define KFD_IOCTL_H_INCLUDED
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define KFD_IOCTL_CURRENT_VERSION 1
> +
> +/* The 64-bit ABI is the authoritative version. */
> +#pragma pack(push, 8)
> +
> +struct kfd_ioctl_get_version_args {
> + uint32_t min_supported_version; /* from KFD */
> + uint32_t max_supported_version; /* from KFD */
> +};
> +
> +/* For kfd_ioctl_create_queue_args.queue_type. */
> +#define KFD_IOC_QUEUE_TYPE_COMPUTE 0
> +#define KFD_IOC_QUEUE_TYPE_SDMA 1
> +
> +struct kfd_ioctl_create_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> + uint32_t ring_size; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> + uint32_t queue_type; /* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority; /* to KFD */
Is this priority global accross all process or local to the process ?
Local is fine. But global is not, if you want some global priority
best is probably to go use some value provided by cgroup.
> + uint64_t write_pointer_address; /* to KFD */
> + uint64_t read_pointer_address; /* to KFD */
> +
> + uint64_t doorbell_address; /* from KFD */
> + uint32_t queue_id; /* from KFD */
> +};
> +
> +struct kfd_ioctl_destroy_queue_args {
> + uint32_t queue_id; /* to KFD */
> +};
> +
> +#define KFD_IOC_MAGIC 'K'
> +
> +#define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> +#define KFD_IOC_CREATE_QUEUE _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
> +#define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> +
> +#pragma pack(pop)
> +
> +#endif
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH 44/83] hsa/radeon: HSA64/HSA32 modes support
From: Jerome Glisse @ 2014-07-11 20:41 UTC (permalink / raw)
To: Oded Gabbay
Cc: David Airlie, Alex Deucher, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, John Bridgman,
Andrew Lewycky, Joerg Roedel, Alexey Skidanov, Oded Gabbay,
Evgeny Pinchuk, Ben Goz, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1405029279-6894-16-git-send-email-oded.gabbay-5C7GfCeVMHo@public.gmane.org>
On Fri, Jul 11, 2014 at 12:54:00AM +0300, Oded Gabbay wrote:
> From: Alexey Skidanov <Alexey.Skidanov-5C7GfCeVMHo@public.gmane.org>
>
> Added apertures initialization and appropriate ioctl
What is process aperture and what it is use for ? This is a very
cryptic commit message.
Cheers,
Jérôme
>
> Signed-off-by: Alexey Skidanov <Alexey.Skidanov-5C7GfCeVMHo@public.gmane.org>
> Signed-off-by: Oded Gabbay <oded.gabbay-5C7GfCeVMHo@public.gmane.org>
> ---
> drivers/gpu/hsa/radeon/Makefile | 2 +-
> drivers/gpu/hsa/radeon/kfd_aperture.c | 124 ++++++++++++++++++++++++++
> drivers/gpu/hsa/radeon/kfd_chardev.c | 58 +++++++++++-
> drivers/gpu/hsa/radeon/kfd_priv.h | 18 ++++
> drivers/gpu/hsa/radeon/kfd_process.c | 17 ++++
> drivers/gpu/hsa/radeon/kfd_sched_cik_static.c | 3 +-
> drivers/gpu/hsa/radeon/kfd_topology.c | 27 ++++++
> include/uapi/linux/kfd_ioctl.h | 18 ++++
> 8 files changed, 264 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/hsa/radeon/kfd_aperture.c
>
> diff --git a/drivers/gpu/hsa/radeon/Makefile b/drivers/gpu/hsa/radeon/Makefile
> index 5422e6a..813b31f 100644
> --- a/drivers/gpu/hsa/radeon/Makefile
> +++ b/drivers/gpu/hsa/radeon/Makefile
> @@ -5,6 +5,6 @@
> radeon_kfd-y := kfd_module.o kfd_device.o kfd_chardev.o \
> kfd_pasid.o kfd_topology.o kfd_process.o \
> kfd_doorbell.o kfd_sched_cik_static.o kfd_registers.o \
> - kfd_vidmem.o kfd_interrupt.o
> + kfd_vidmem.o kfd_interrupt.o kfd_aperture.o
>
> obj-$(CONFIG_HSA_RADEON) += radeon_kfd.o
> diff --git a/drivers/gpu/hsa/radeon/kfd_aperture.c b/drivers/gpu/hsa/radeon/kfd_aperture.c
> new file mode 100644
> index 0000000..9e2d6da
> --- /dev/null
> +++ b/drivers/gpu/hsa/radeon/kfd_aperture.c
> @@ -0,0 +1,124 @@
> +/*
> + * Copyright 2014 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/compat.h>
> +#include <uapi/linux/kfd_ioctl.h>
> +#include <linux/time.h>
> +#include "kfd_priv.h"
> +#include "kfd_scheduler.h"
> +#include <linux/mm.h>
> +#include <uapi/asm-generic/mman-common.h>
> +#include <asm/processor.h>
> +
> +
> +#define MAKE_GPUVM_APP_BASE(gpu_num) (((uint64_t)(gpu_num) << 61) + 0x1000000000000)
> +#define MAKE_GPUVM_APP_LIMIT(base) (((uint64_t)(base) & 0xFFFFFF0000000000) | 0xFFFFFFFFFF)
> +#define MAKE_SCRATCH_APP_BASE(gpu_num) (((uint64_t)(gpu_num) << 61) + 0x100000000)
> +#define MAKE_SCRATCH_APP_LIMIT(base) (((uint64_t)base & 0xFFFFFFFF00000000) | 0xFFFFFFFF)
> +#define MAKE_LDS_APP_BASE(gpu_num) (((uint64_t)(gpu_num) << 61) + 0x0)
> +#define MAKE_LDS_APP_LIMIT(base) (((uint64_t)(base) & 0xFFFFFFFF00000000) | 0xFFFFFFFF)
> +
> +#define HSA_32BIT_LDS_APP_SIZE 0x10000
> +#define HSA_32BIT_LDS_APP_ALIGNMENT 0x10000
> +
> +static unsigned long kfd_reserve_aperture(struct kfd_process *process, unsigned long len, unsigned long alignment)
> +{
> +
> + unsigned long addr = 0;
> + unsigned long start_address;
> +
> + /*
> + * Go bottom up and find the first available aligned address.
> + * We may narrow space to scan by getting mmap range limits.
> + */
> + for (start_address = alignment; start_address < (TASK_SIZE - alignment); start_address += alignment) {
> + addr = vm_mmap(NULL, start_address, len, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0);
> + if (!IS_ERR_VALUE(addr)) {
> + if (addr == start_address)
> + return addr;
> + vm_munmap(addr, len);
> + }
> + }
> + return 0;
> +
> +}
> +
> +int kfd_init_apertures(struct kfd_process *process)
> +{
> + uint8_t id = 0;
> + struct kfd_dev *dev;
> + struct kfd_process_device *pdd;
> +
> + mutex_lock(&process->mutex);
> +
> + /*Iterating over all devices*/
> + while ((dev = kfd_topology_enum_kfd_devices(id)) != NULL && id < NUM_OF_SUPPORTED_GPUS) {
> +
> + pdd = radeon_kfd_get_process_device_data(dev, process);
> +
> + /*for 64 bit process aperture will be statically reserved in the non canonical process address space
> + *for 32 bit process the aperture will be reserved in the process address space
> + */
> + if (process->is_32bit_user_mode) {
> + /*try to reserve aperture. continue on failure, just put the aperture size to be 0*/
> + pdd->lds_base = kfd_reserve_aperture(
> + process,
> + HSA_32BIT_LDS_APP_SIZE,
> + HSA_32BIT_LDS_APP_ALIGNMENT);
> +
> + if (pdd->lds_base)
> + pdd->lds_limit = pdd->lds_base + HSA_32BIT_LDS_APP_SIZE - 1;
> + else
> + pdd->lds_limit = 0;
> +
> + /*GPUVM and Scratch apertures are not supported*/
> + pdd->gpuvm_base = pdd->gpuvm_limit = pdd->scratch_base = pdd->scratch_limit = 0;
> + } else {
> + /*node id couldn't be 0 - the three MSB bits of aperture shoudn't be 0*/
> + pdd->lds_base = MAKE_LDS_APP_BASE(id + 1);
> + pdd->lds_limit = MAKE_LDS_APP_LIMIT(pdd->lds_base);
> + pdd->gpuvm_base = MAKE_GPUVM_APP_BASE(id + 1);
> + pdd->gpuvm_limit = MAKE_GPUVM_APP_LIMIT(pdd->gpuvm_base);
> + pdd->scratch_base = MAKE_SCRATCH_APP_BASE(id + 1);
> + pdd->scratch_limit = MAKE_SCRATCH_APP_LIMIT(pdd->scratch_base);
> + }
> +
> + dev_dbg(kfd_device, "node id %u, gpu id %u, lds_base %llX lds_limit %llX gpuvm_base %llX gpuvm_limit %llX scratch_base %llX scratch_limit %llX",
> + id, pdd->dev->id, pdd->lds_base, pdd->lds_limit, pdd->gpuvm_base, pdd->gpuvm_limit, pdd->scratch_base, pdd->scratch_limit);
> +
> + id++;
> + }
> +
> + mutex_unlock(&process->mutex);
> +
> + return 0;
> +}
> +
> +
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index e95d597..07cac88 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -32,6 +32,9 @@
> #include <linux/time.h>
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
> +#include <linux/mm.h>
> +#include <uapi/asm-generic/mman-common.h>
> +#include <asm/processor.h>
>
> static long kfd_ioctl(struct file *, unsigned int, unsigned long);
> static int kfd_open(struct inode *, struct file *);
> @@ -107,9 +110,13 @@ kfd_open(struct inode *inode, struct file *filep)
> process = radeon_kfd_create_process(current);
> if (IS_ERR(process))
> return PTR_ERR(process);
> +
> process->is_32bit_user_mode = is_compat_task();
> +
> dev_info(kfd_device, "process %d opened, compat mode (32 bit) - %d\n",
> - process->pasid, process->is_32bit_user_mode);
> + process->pasid, process->is_32bit_user_mode);
> +
> + kfd_init_apertures(process);
>
> return 0;
> }
> @@ -321,6 +328,51 @@ kfd_ioctl_get_clock_counters(struct file *filep, struct kfd_process *p, void __u
> return 0;
> }
>
> +
> +static int kfd_ioctl_get_process_apertures(struct file *filp, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_get_process_apertures_args args;
> + struct kfd_process_device *pdd;
> +
> + dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + args.num_of_nodes = 0;
> +
> + mutex_lock(&p->mutex);
> +
> + /*if the process-device list isn't empty*/
> + if (kfd_has_process_device_data(p)) {
> + /* Run over all pdd of the process */
> + pdd = kfd_get_first_process_device_data(p);
> + do {
> +
> + args.process_apertures[args.num_of_nodes].gpu_id = pdd->dev->id;
> + args.process_apertures[args.num_of_nodes].lds_base = pdd->lds_base;
> + args.process_apertures[args.num_of_nodes].lds_limit = pdd->lds_limit;
> + args.process_apertures[args.num_of_nodes].gpuvm_base = pdd->gpuvm_base;
> + args.process_apertures[args.num_of_nodes].gpuvm_limit = pdd->gpuvm_limit;
> + args.process_apertures[args.num_of_nodes].scratch_base = pdd->scratch_base;
> + args.process_apertures[args.num_of_nodes].scratch_limit = pdd->scratch_limit;
> +
> + dev_dbg(kfd_device, "node id %u, gpu id %u, lds_base %llX lds_limit %llX gpuvm_base %llX gpuvm_limit %llX scratch_base %llX scratch_limit %llX",
> + args.num_of_nodes, pdd->dev->id, pdd->lds_base, pdd->lds_limit, pdd->gpuvm_base, pdd->gpuvm_limit, pdd->scratch_base, pdd->scratch_limit);
> + args.num_of_nodes++;
> + } while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
> + (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
> + }
> +
> + mutex_unlock(&p->mutex);
> +
> + if (copy_to_user(arg, &args, sizeof(args)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +
> static long
> kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> {
> @@ -352,6 +404,10 @@ kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> err = kfd_ioctl_get_clock_counters(filep, process, (void __user *)arg);
> break;
>
> + case KFD_IOC_GET_PROCESS_APERTURES:
> + err = kfd_ioctl_get_process_apertures(filep, process, (void __user *)arg);
> + break;
> +
> default:
> dev_err(kfd_device,
> "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
> diff --git a/drivers/gpu/hsa/radeon/kfd_priv.h b/drivers/gpu/hsa/radeon/kfd_priv.h
> index 9d3b1fc..28155bc 100644
> --- a/drivers/gpu/hsa/radeon/kfd_priv.h
> +++ b/drivers/gpu/hsa/radeon/kfd_priv.h
> @@ -171,6 +171,16 @@ struct kfd_process_device {
>
> /* Is this process/pasid bound to this device? (amd_iommu_bind_pasid) */
> bool bound;
> +
> + /*Apertures*/
> + uint64_t lds_base;
> + uint64_t lds_limit;
> + uint64_t gpuvm_base;
> + uint64_t gpuvm_limit;
> + uint64_t scratch_base;
> + uint64_t scratch_limit;
> +
> +
> };
>
> /* Process data */
> @@ -212,6 +222,10 @@ void radeon_kfd_install_queue(struct kfd_process *p, unsigned int queue_id, stru
> void radeon_kfd_remove_queue(struct kfd_process *p, unsigned int queue_id);
> struct kfd_queue *radeon_kfd_get_queue(struct kfd_process *p, unsigned int queue_id);
>
> +/* Process device data iterator */
> +struct kfd_process_device *kfd_get_first_process_device_data(struct kfd_process *p);
> +struct kfd_process_device *kfd_get_next_process_device_data(struct kfd_process *p, struct kfd_process_device *pdd);
> +bool kfd_has_process_device_data(struct kfd_process *p);
>
> /* PASIDs */
> int radeon_kfd_pasid_init(void);
> @@ -237,6 +251,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu);
> int kfd_topology_remove_device(struct kfd_dev *gpu);
> struct kfd_dev *radeon_kfd_device_by_id(uint32_t gpu_id);
> struct kfd_dev *radeon_kfd_device_by_pci_dev(const struct pci_dev *pdev);
> +struct kfd_dev *kfd_topology_enum_kfd_devices(uint8_t idx);
>
> /* MMIO registers */
> #define WRITE_REG(dev, reg, value) radeon_kfd_write_reg((dev), (reg), (value))
> @@ -253,4 +268,7 @@ void kgd2kfd_interrupt(struct kfd_dev *dev, const void *ih_ring_entry);
> void kgd2kfd_suspend(struct kfd_dev *dev);
> int kgd2kfd_resume(struct kfd_dev *dev);
>
> +/*HSA apertures*/
> +int kfd_init_apertures(struct kfd_process *process);
> +
> #endif
> diff --git a/drivers/gpu/hsa/radeon/kfd_process.c b/drivers/gpu/hsa/radeon/kfd_process.c
> index f89f855..80136e6 100644
> --- a/drivers/gpu/hsa/radeon/kfd_process.c
> +++ b/drivers/gpu/hsa/radeon/kfd_process.c
> @@ -397,3 +397,20 @@ struct kfd_queue *radeon_kfd_get_queue(struct kfd_process *p, unsigned int queue
> test_bit(queue_id, p->allocated_queue_bitmap)) ?
> p->queues[queue_id] : NULL;
> }
> +
> +struct kfd_process_device *kfd_get_first_process_device_data(struct kfd_process *p)
> +{
> + return list_first_entry(&p->per_device_data, struct kfd_process_device, per_device_list);
> +}
> +
> +struct kfd_process_device *kfd_get_next_process_device_data(struct kfd_process *p, struct kfd_process_device *pdd)
> +{
> + if (list_is_last(&pdd->per_device_list, &p->per_device_data))
> + return NULL;
> + return list_next_entry(pdd, per_device_list);
> +}
> +
> +bool kfd_has_process_device_data(struct kfd_process *p)
> +{
> + return !(list_empty(&p->per_device_data));
> +}
> diff --git a/drivers/gpu/hsa/radeon/kfd_sched_cik_static.c b/drivers/gpu/hsa/radeon/kfd_sched_cik_static.c
> index 7ee8125..30561a6 100644
> --- a/drivers/gpu/hsa/radeon/kfd_sched_cik_static.c
> +++ b/drivers/gpu/hsa/radeon/kfd_sched_cik_static.c
> @@ -627,7 +627,8 @@ static void cik_static_deregister_process(struct kfd_scheduler *scheduler,
> struct cik_static_private *priv = kfd_scheduler_to_private(scheduler);
> struct cik_static_process *pp = kfd_process_to_private(scheduler_process);
>
> - if (priv && pp) {
> +
> + if (priv && pp) {
> release_vmid(priv, pp->vmid);
> kfree(pp);
> }
> diff --git a/drivers/gpu/hsa/radeon/kfd_topology.c b/drivers/gpu/hsa/radeon/kfd_topology.c
> index 21bb66e..213ae7b 100644
> --- a/drivers/gpu/hsa/radeon/kfd_topology.c
> +++ b/drivers/gpu/hsa/radeon/kfd_topology.c
> @@ -1201,3 +1201,30 @@ int kfd_topology_remove_device(struct kfd_dev *gpu)
>
> return res;
> }
> +
> +/*
> + * When idx is out of bounds, the function will return NULL
> + */
> +struct kfd_dev *kfd_topology_enum_kfd_devices(uint8_t idx)
> +{
> +
> + struct kfd_topology_device *top_dev;
> + struct kfd_dev *device = NULL;
> + uint8_t device_idx = 0;
> +
> + down_read(&topology_lock);
> +
> + list_for_each_entry(top_dev, &topology_device_list, list) {
> + if (device_idx == idx) {
> + device = top_dev->gpu;
> + break;
> + }
> +
> + device_idx++;
> + }
> +
> + up_read(&topology_lock);
> +
> + return device;
> +
> +}
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index a7c3abd..e5fcb8b 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -78,6 +78,23 @@ struct kfd_ioctl_get_clock_counters_args {
> uint64_t system_clock_freq; /* from KFD */
> };
>
> +#define NUM_OF_SUPPORTED_GPUS 7
> +
> +struct kfd_process_device_apertures {
> + uint64_t lds_base;/* from KFD */
> + uint64_t lds_limit;/* from KFD */
> + uint64_t scratch_base;/* from KFD */
> + uint64_t scratch_limit;/* from KFD */
> + uint64_t gpuvm_base;/* from KFD */
> + uint64_t gpuvm_limit;/* from KFD */
> + uint32_t gpu_id;/* from KFD */
> +};
> +
> +struct kfd_ioctl_get_process_apertures_args {
> + struct kfd_process_device_apertures process_apertures[NUM_OF_SUPPORTED_GPUS];/* from KFD */
> + uint8_t num_of_nodes; /* from KFD, should be in the range [1 - NUM_OF_SUPPORTED_GPUS]*/
> +};
> +
> #define KFD_IOC_MAGIC 'K'
>
> #define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> @@ -85,6 +102,7 @@ struct kfd_ioctl_get_clock_counters_args {
> #define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> #define KFD_IOC_SET_MEMORY_POLICY _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
> #define KFD_IOC_GET_CLOCK_COUNTERS _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
> +#define KFD_IOC_GET_PROCESS_APERTURES _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
>
> #pragma pack(pop)
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH 32/83] hsa/radeon: implementing IOCTL for clock counters
From: Jerome Glisse @ 2014-07-11 20:34 UTC (permalink / raw)
To: Oded Gabbay
Cc: Andrew Lewycky, Ben Goz, linux-kernel, dri-devel, Evgeny Pinchuk,
Alexey Skidanov, linux-api, Alex Deucher
In-Reply-To: <1405029279-6894-4-git-send-email-oded.gabbay@amd.com>
On Fri, Jul 11, 2014 at 12:53:48AM +0300, Oded Gabbay wrote:
> From: Evgeny Pinchuk <evgeny.pinchuk@amd.com>
>
> Implemented new IOCTL to query the CPU and GPU clock counters.
>
> Signed-off-by: Evgeny Pinchuk <evgeny.pinchuk@amd.com>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
> ---
> drivers/gpu/hsa/radeon/kfd_chardev.c | 37 ++++++++++++++++++++++++++++++++++++
> include/uapi/linux/kfd_ioctl.h | 9 +++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index ddaf357..d6fa980 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -28,6 +28,7 @@
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <uapi/linux/kfd_ioctl.h>
> +#include <linux/time.h>
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
>
> @@ -284,6 +285,38 @@ out:
> return err;
> }
>
> +static long
> +kfd_ioctl_get_clock_counters(struct file *filep, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_get_clock_counters_args args;
> + struct kfd_dev *dev;
> + struct timespec time;
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + dev = radeon_kfd_device_by_id(args.gpu_id);
> + if (dev == NULL)
> + return -EINVAL;
> +
> + /* Reading GPU clock counter from KGD */
> + args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
> +
> + /* No access to rdtsc. Using raw monotonic time */
> + getrawmonotonic(&time);
> + args.cpu_clock_counter = time.tv_nsec;
Is the GPU clock counter monotonic too ? Even after GPU reset (hard reset
included) what could go wrong if it rolls back ?
> +
> + get_monotonic_boottime(&time);
> + args.system_clock_counter = time.tv_nsec;
> +
> + /* Since the counter is in nano-seconds we use 1GHz frequency */
> + args.system_clock_freq = 1000000000;
> +
> + if (copy_to_user(arg, &args, sizeof(args)))
> + return -EFAULT;
> +
> + return 0;
> +}
>
> static long
> kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> @@ -312,6 +345,10 @@ kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> err = kfd_ioctl_set_memory_policy(filep, process, (void __user *)arg);
> break;
>
> + case KFD_IOC_GET_CLOCK_COUNTERS:
> + err = kfd_ioctl_get_clock_counters(filep, process, (void __user *)arg);
> + break;
> +
> default:
> dev_err(kfd_device,
> "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 928e628..5b9517e 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -70,12 +70,21 @@ struct kfd_ioctl_set_memory_policy_args {
> uint64_t alternate_aperture_size; /* to KFD */
> };
>
> +struct kfd_ioctl_get_clock_counters_args {
> + uint32_t gpu_id; /* to KFD */
> + uint64_t gpu_clock_counter; /* from KFD */
> + uint64_t cpu_clock_counter; /* from KFD */
> + uint64_t system_clock_counter; /* from KFD */
> + uint64_t system_clock_freq; /* from KFD */
> +};
> +
> #define KFD_IOC_MAGIC 'K'
>
> #define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> #define KFD_IOC_CREATE_QUEUE _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
> #define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> #define KFD_IOC_SET_MEMORY_POLICY _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
> +#define KFD_IOC_GET_CLOCK_COUNTERS _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
>
> #pragma pack(pop)
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH 13/83] hsa/radeon: Add 2 new IOCTL to kfd, CREATE_QUEUE and DESTROY_QUEUE
From: Jerome Glisse @ 2014-07-11 19:19 UTC (permalink / raw)
To: Oded Gabbay
Cc: Andrew Lewycky, Ben Goz, linux-kernel, dri-devel, Evgeny Pinchuk,
Alexey Skidanov, linux-api, Alex Deucher
In-Reply-To: <1405029027-6085-12-git-send-email-oded.gabbay@amd.com>
On Fri, Jul 11, 2014 at 12:50:13AM +0300, Oded Gabbay wrote:
> This patch adds 2 new IOCTL to kfd driver.
>
> The first IOCTL is KFD_IOC_CREATE_QUEUE that is used by the user-mode
> application to create a compute queue on the GPU.
>
> The second IOCTL is KFD_IOC_DESTROY_QUEUE that is used by the
> user-mode application to destroy an existing compute queue on the GPU.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
Coding style need fixing. What is the percent argument ? What is it use
for ?
You need to check range validity of argument provided by userspace. Rules
is never trust userspace. Especialy for things like queue_size which is
use without never being check allowing userspace to send 0 which leads
to broken queue size.
Also out of curiosity what kind of event happens if userspace munmap its
ring buffer before unregistering a queue ?
> ---
> drivers/gpu/hsa/radeon/kfd_chardev.c | 155 ++++++++++++++++++++++++++++++++++
> drivers/gpu/hsa/radeon/kfd_doorbell.c | 11 +++
> include/uapi/linux/kfd_ioctl.h | 69 +++++++++++++++
Again better to create an hsa directory for kfd_ioctl.h
> 3 files changed, 235 insertions(+)
> create mode 100644 include/uapi/linux/kfd_ioctl.h
>
> diff --git a/drivers/gpu/hsa/radeon/kfd_chardev.c b/drivers/gpu/hsa/radeon/kfd_chardev.c
> index 0b5bc74..4e7d5d0 100644
> --- a/drivers/gpu/hsa/radeon/kfd_chardev.c
> +++ b/drivers/gpu/hsa/radeon/kfd_chardev.c
> @@ -27,11 +27,13 @@
> #include <linux/sched.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <uapi/linux/kfd_ioctl.h>
> #include "kfd_priv.h"
> #include "kfd_scheduler.h"
>
> static long kfd_ioctl(struct file *, unsigned int, unsigned long);
> static int kfd_open(struct inode *, struct file *);
> +static int kfd_mmap(struct file *, struct vm_area_struct *);
>
> static const char kfd_dev_name[] = "kfd";
>
> @@ -108,17 +110,170 @@ kfd_open(struct inode *inode, struct file *filep)
> return 0;
> }
>
> +static long
> +kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_create_queue_args args;
> + struct kfd_dev *dev;
> + int err = 0;
> + unsigned int queue_id;
> + struct kfd_queue *queue;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + dev = radeon_kfd_device_by_id(args.gpu_id);
> + if (dev == NULL)
> + return -EINVAL;
> +
> + queue = kzalloc(
> + offsetof(struct kfd_queue, scheduler_queue) + dev->device_info->scheduler_class->queue_size,
> + GFP_KERNEL);
> +
> + if (!queue)
> + return -ENOMEM;
> +
> + queue->dev = dev;
> +
> + mutex_lock(&p->mutex);
> +
> + pdd = radeon_kfd_bind_process_to_device(dev, p);
> + if (IS_ERR(pdd) < 0) {
> + err = PTR_ERR(pdd);
> + goto err_bind_pasid;
> + }
> +
> + pr_debug("kfd: creating queue number %d for PASID %d on GPU 0x%x\n",
> + pdd->queue_count,
> + p->pasid,
> + dev->id);
> +
> + if (pdd->queue_count++ == 0) {
> + err = dev->device_info->scheduler_class->register_process(dev->scheduler, p, &pdd->scheduler_process);
> + if (err < 0)
> + goto err_register_process;
> + }
> +
> + if (!radeon_kfd_allocate_queue_id(p, &queue_id))
> + goto err_allocate_queue_id;
> +
> + err = dev->device_info->scheduler_class->create_queue(dev->scheduler, pdd->scheduler_process,
> + &queue->scheduler_queue,
> + (void __user *)args.ring_base_address,
> + args.ring_size,
> + (void __user *)args.read_pointer_address,
> + (void __user *)args.write_pointer_address,
> + radeon_kfd_queue_id_to_doorbell(dev, p, queue_id));
> + if (err)
> + goto err_create_queue;
> +
> + radeon_kfd_install_queue(p, queue_id, queue);
> +
> + args.queue_id = queue_id;
> + args.doorbell_address = (uint64_t)(uintptr_t)radeon_kfd_get_doorbell(filep, p, dev, queue_id);
> +
> + if (copy_to_user(arg, &args, sizeof(args))) {
> + err = -EFAULT;
> + goto err_copy_args_out;
> + }
> +
> + mutex_unlock(&p->mutex);
> +
> + pr_debug("kfd: queue id %d was created successfully.\n"
> + " ring buffer address == 0x%016llX\n"
> + " read ptr address == 0x%016llX\n"
> + " write ptr address == 0x%016llX\n"
> + " doorbell address == 0x%016llX\n",
> + args.queue_id,
> + args.ring_base_address,
> + args.read_pointer_address,
> + args.write_pointer_address,
> + args.doorbell_address);
> +
> + return 0;
> +
> +err_copy_args_out:
> + dev->device_info->scheduler_class->destroy_queue(dev->scheduler, &queue->scheduler_queue);
> +err_create_queue:
> + radeon_kfd_remove_queue(p, queue_id);
> +err_allocate_queue_id:
> + if (--pdd->queue_count == 0) {
> + dev->device_info->scheduler_class->deregister_process(dev->scheduler, pdd->scheduler_process);
> + pdd->scheduler_process = NULL;
> + }
> +err_register_process:
> +err_bind_pasid:
> + kfree(queue);
> + mutex_unlock(&p->mutex);
> + return err;
> +}
> +
> +static int
> +kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p, void __user *arg)
> +{
> + struct kfd_ioctl_destroy_queue_args args;
> + struct kfd_queue *queue;
> + struct kfd_dev *dev;
> + struct kfd_process_device *pdd;
> +
> + if (copy_from_user(&args, arg, sizeof(args)))
> + return -EFAULT;
> +
> + mutex_lock(&p->mutex);
> +
> + queue = radeon_kfd_get_queue(p, args.queue_id);
> + if (!queue) {
> + mutex_unlock(&p->mutex);
> + return -EINVAL;
> + }
> +
> + dev = queue->dev;
> +
> + pr_debug("kfd: destroying queue id %d for PASID %d\n",
> + args.queue_id,
> + p->pasid);
> +
> + radeon_kfd_remove_queue(p, args.queue_id);
> + dev->device_info->scheduler_class->destroy_queue(dev->scheduler, &queue->scheduler_queue);
> +
> + kfree(queue);
> +
> + pdd = radeon_kfd_get_process_device_data(dev, p);
> + BUG_ON(pdd == NULL); /* Because a queue exists. */
> +
> + if (--pdd->queue_count == 0) {
> + dev->device_info->scheduler_class->deregister_process(dev->scheduler, pdd->scheduler_process);
> + pdd->scheduler_process = NULL;
> + }
> +
> + mutex_unlock(&p->mutex);
> + return 0;
> +}
>
> static long
> kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> {
> + struct kfd_process *process;
> long err = -EINVAL;
>
> dev_info(kfd_device,
> "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
> cmd, _IOC_NR(cmd), arg);
>
> + process = radeon_kfd_get_process(current);
> + if (IS_ERR(process))
> + return PTR_ERR(process);
> +
> switch (cmd) {
> + case KFD_IOC_CREATE_QUEUE:
> + err = kfd_ioctl_create_queue(filep, process, (void __user *)arg);
> + break;
> +
> + case KFD_IOC_DESTROY_QUEUE:
> + err = kfd_ioctl_destroy_queue(filep, process, (void __user *)arg);
> + break;
> +
> default:
> dev_err(kfd_device,
> "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
> diff --git a/drivers/gpu/hsa/radeon/kfd_doorbell.c b/drivers/gpu/hsa/radeon/kfd_doorbell.c
> index e1d8506..3de8a02 100644
> --- a/drivers/gpu/hsa/radeon/kfd_doorbell.c
> +++ b/drivers/gpu/hsa/radeon/kfd_doorbell.c
> @@ -155,3 +155,14 @@ doorbell_t __user *radeon_kfd_get_doorbell(struct file *devkfd, struct kfd_proce
> return &pdd->doorbell_mapping[doorbell_index];
> }
>
> +/*
> + * queue_ids are in the range [0,MAX_PROCESS_QUEUES) and are mapped 1:1
> + * to doorbells with the process's doorbell page
> + */
> +unsigned int radeon_kfd_queue_id_to_doorbell(struct kfd_dev *kfd, struct kfd_process *process, unsigned int queue_id)
> +{
> + /* doorbell_id_offset accounts for doorbells taken by KGD.
> + * pasid * doorbell_process_allocation/sizeof(doorbell_t) adjusts to the process's doorbells */
> + return kfd->doorbell_id_offset + process->pasid * (doorbell_process_allocation()/sizeof(doorbell_t)) + queue_id;
> +}
> +
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> new file mode 100644
> index 0000000..dcc5fe0
> --- /dev/null
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright 2014 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef KFD_IOCTL_H_INCLUDED
> +#define KFD_IOCTL_H_INCLUDED
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define KFD_IOCTL_CURRENT_VERSION 1
> +
> +/* The 64-bit ABI is the authoritative version. */
> +#pragma pack(push, 8)
> +
> +struct kfd_ioctl_get_version_args {
> + uint32_t min_supported_version; /* from KFD */
> + uint32_t max_supported_version; /* from KFD */
> +};
> +
> +/* For kfd_ioctl_create_queue_args.queue_type. */
> +#define KFD_IOC_QUEUE_TYPE_COMPUTE 0
> +#define KFD_IOC_QUEUE_TYPE_SDMA 1
> +
> +struct kfd_ioctl_create_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> + uint32_t ring_size; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> + uint32_t queue_type; /* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority; /* to KFD */
> + uint64_t write_pointer_address; /* to KFD */
> + uint64_t read_pointer_address; /* to KFD */
> +
> + uint64_t doorbell_address; /* from KFD */
> + uint32_t queue_id; /* from KFD */
> +};
> +
> +struct kfd_ioctl_destroy_queue_args {
> + uint32_t queue_id; /* to KFD */
> +};
> +
> +#define KFD_IOC_MAGIC 'K'
> +
> +#define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> +#define KFD_IOC_CREATE_QUEUE _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
> +#define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> +
> +#pragma pack(pop)
> +
> +#endif
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-11 17:55 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Andy Lutomirski, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <20140711164931.GA18473@redhat.com>
On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/10, Kees Cook wrote:
>>
>> This adds the ability for threads to request seccomp filter
>> synchronization across their thread group (at filter attach time).
>> For example, for Chrome to make sure graphic driver threads are fully
>> confined after seccomp filters have been attached.
>>
>> To support this, locking on seccomp changes via thread-group-shared
>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>> with thread creation are handled via delayed duplication of the seccomp
>> task struct field and cred_guard_mutex.
>>
>> This includes a new syscall (instead of adding a new prctl option),
>> as suggested by Andy Lutomirski and Michael Kerrisk.
>
> I do not not see any problems in this version,
Awesome! Thank you for all the reviews. :) If Andy and Michael are
happy with this too, I think this is in good shape. \o/
-Kees
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
--
Kees Cook
Chrome OS Security
^ 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