| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANq1E4QivtLBoLz+0quKh5gxP2R0xwhCAA-oBdF_GTTRASzUcw@mail.gmail.com>
Date: Sat, 19 Jul 2014 18:29:30 +0200
From: David Herrmann <dh.herrmann@...il.com>
To: Hugh Dickins <hughd@...gle.com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
Michael Kerrisk <mtk.manpages@...il.com>,
Ryan Lortie <desrt@...rt.ca>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-mm <linux-mm@...ck.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
Linux API <linux-api@...r.kernel.org>,
Greg Kroah-Hartman <greg@...ah.com>,
John Stultz <john.stultz@...aro.org>,
Lennart Poettering <lennart@...ttering.net>,
Daniel Mack <zonque@...il.com>, Kay Sievers <kay@...y.org>,
Tony Battersby <tonyb@...ernetics.com>,
Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH v3 3/7] shm: add memfd_create() syscall
Hi
On Wed, Jul 16, 2014 at 12:07 PM, Hugh Dickins <hughd@...gle.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> memfd_create() is similar to mmap(MAP_ANON), but returns a file-descriptor
>> that you can pass to mmap(). It can support sealing and avoids any
>> connection to user-visible mount-points. Thus, it's not subject to quotas
>> on mounted file-systems, but can be used like malloc()'ed memory, but
>> with a file-descriptor to it.
>>
>> memfd_create() returns the raw shmem file, so calls like ftruncate() can
>> be used to modify the underlying inode. Also calls like fstat()
>> will return proper information and mark the file as regular file. If you
>> want sealing, you can specify MFD_ALLOW_SEALING. Otherwise, sealing is not
>> supported (like on all other regular files).
>>
>> Compared to O_TMPFILE, it does not require a tmpfs mount-point and is not
>> subject to quotas and alike. It is still properly accounted to memcg
>> limits, though.
>
> It's an important point, but unclear quite what "quotas and alike" means.
> There's never been any quota support in shmem/tmpfs, but filesystem size
> can be limited. Maybe say "and is not subject to a filesystem size limit.
> It is still properly accounted to memcg limits, though, and to the same
> overcommit or no-overcommit accounting as all user memory."
Yes, makes sense. Fixed.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@...il.com>
>
> A comment or two below, but this is okay by me. I'm not wildly excited
> to be getting a new system call in mm/shmem.c. I do like it much better
> now that you've dropped the size arg, thank you, but I still find it an
> odd system call: if it were not for the name, that you want so much for
> debugging, I think we would just implement this with a /dev/sealable
> alongside /dev/zero, which gave you your own object on opening (in the
> way that /dev/zero gives you your own object on mmap'ing).
mmap() supports replacing the file by a new file. Therefore, /dev/zero
works just fine. open() doesn't allow that and it looks non-trivial to
make it work. "non-trivial" is not really a counter-argument, but the
object-name is worth a new syscall, in my opinion. And it's a really
nice feature to debug complex systems.
> I haven't checked the manpage, I hope it's made very clear that
> there's no uniqueness imposed on the name, that it's merely a tag
> attached to the object.
Yes, the man-page clearly states that names are for debugging purposes
only and exposed via /proc/self/fd/ symlink-targets. They're not
subject to conflict-tests nor do two memfd's with the same name behave
any different.
> But from a shmem point of view this seems fine: if everyone else
> is happy with memfd_create(), it's fine by me.
>
>> ---
>> arch/x86/syscalls/syscall_32.tbl | 1 +
>> arch/x86/syscalls/syscall_64.tbl | 1 +
>> include/linux/syscalls.h | 1 +
>> include/uapi/linux/memfd.h | 8 +++++
>> kernel/sys_ni.c | 1 +
>> mm/shmem.c | 72 ++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 84 insertions(+)
>> create mode 100644 include/uapi/linux/memfd.h
>>
>> diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
>> index d6b8679..e7495b4 100644
>> --- a/arch/x86/syscalls/syscall_32.tbl
>> +++ b/arch/x86/syscalls/syscall_32.tbl
>> @@ -360,3 +360,4 @@
>> 351 i386 sched_setattr sys_sched_setattr
>> 352 i386 sched_getattr sys_sched_getattr
>> 353 i386 renameat2 sys_renameat2
>> +354 i386 memfd_create sys_memfd_create
>> diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
>> index ec255a1..28be0e1 100644
>> --- a/arch/x86/syscalls/syscall_64.tbl
>> +++ b/arch/x86/syscalls/syscall_64.tbl
>> @@ -323,6 +323,7 @@
>> 314 common sched_setattr sys_sched_setattr
>> 315 common sched_getattr sys_sched_getattr
>> 316 common renameat2 sys_renameat2
>> +317 common memfd_create sys_memfd_create
>>
>> #
>> # x32-specific system call numbers start at 512 to avoid cache impact
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index b0881a0..0be5d4d 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -802,6 +802,7 @@ asmlinkage long sys_timerfd_settime(int ufd, int flags,
>> asmlinkage long sys_timerfd_gettime(int ufd, struct itimerspec __user *otmr);
>> asmlinkage long sys_eventfd(unsigned int count);
>> asmlinkage long sys_eventfd2(unsigned int count, int flags);
>> +asmlinkage long sys_memfd_create(const char *uname_ptr, unsigned int flags);
>> asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
>> asmlinkage long sys_old_readdir(unsigned int, struct old_linux_dirent __user *, unsigned int);
>> asmlinkage long sys_pselect6(int, fd_set __user *, fd_set __user *,
>> diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
>> new file mode 100644
>> index 0000000..534e364
>> --- /dev/null
>> +++ b/include/uapi/linux/memfd.h
>> @@ -0,0 +1,8 @@
>> +#ifndef _UAPI_LINUX_MEMFD_H
>> +#define _UAPI_LINUX_MEMFD_H
>> +
>> +/* flags for memfd_create(2) (unsigned int) */
>> +#define MFD_CLOEXEC 0x0001U
>> +#define MFD_ALLOW_SEALING 0x0002U
>> +
>> +#endif /* _UAPI_LINUX_MEMFD_H */
>> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
>> index 36441b5..489a4e6 100644
>> --- a/kernel/sys_ni.c
>> +++ b/kernel/sys_ni.c
>> @@ -197,6 +197,7 @@ cond_syscall(compat_sys_timerfd_settime);
>> cond_syscall(compat_sys_timerfd_gettime);
>> cond_syscall(sys_eventfd);
>> cond_syscall(sys_eventfd2);
>> +cond_syscall(sys_memfd_create);
>>
>> /* performance counters: */
>> cond_syscall(sys_perf_event_open);
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 1438b3e..e7c5fe1 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -66,7 +66,9 @@ static struct vfsmount *shm_mnt;
>> #include <linux/highmem.h>
>> #include <linux/seq_file.h>
>> #include <linux/magic.h>
>> +#include <linux/syscalls.h>
>> #include <linux/fcntl.h>
>> +#include <uapi/linux/memfd.h>
>>
>> #include <asm/uaccess.h>
>> #include <asm/pgtable.h>
>> @@ -2662,6 +2664,76 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>> shmem_show_mpol(seq, sbinfo->mpol);
>> return 0;
>> }
>> +
>> +#define MFD_NAME_PREFIX "memfd:"
>> +#define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1)
>> +#define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
>> +
>> +#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING)
>> +
>> +SYSCALL_DEFINE2(memfd_create,
>> + const char*, uname,
>
> Jann Horn suggested "const char __user *" rather than "const char *",
> here and in syscalls.h, I think that's right (for sparse: compare
> with sys_open, for example).
Both fixed already. Sorry, I forgot to reply to Jann Horn. Thanks to
both of you!
>> + unsigned int, flags)
>> +{
>> + struct shmem_inode_info *info;
>> + struct file *file;
>> + int fd, error;
>> + char *name;
>> + long len;
>> +
>> + if (flags & ~(unsigned int)MFD_ALL_FLAGS)
>> + return -EINVAL;
>> +
>> + /* length includes terminating zero */
>> + len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
>> + if (len <= 0)
>> + return -EFAULT;
>> + if (len > MFD_NAME_MAX_LEN + 1)
>> + return -EINVAL;
>> +
>> + name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_TEMPORARY);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + strcpy(name, MFD_NAME_PREFIX);
>> + if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
>> + error = -EFAULT;
>> + goto err_name;
>> + }
>> +
>> + /* terminating-zero may have changed after strnlen_user() returned */
>> + if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
>> + error = -EFAULT;
>> + goto err_name;
>> + }
>> +
>> + fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
>
> Perhaps we should throw O_LARGEFILE in there too? So 32-bit is not
> surprised when it accesses beyond MAX_NON_LFS. I guess it's almost
> a non-issue, since the file is in memory, so not expected to be very
> large; but I seem to recall being caught out by a missing O_LARGEFILE
> in the past, and a new interface like this might do better to force it.
>
> But I'm not very sure of my ground here: please ask around, an fsdevel
> person will have a better idea than me, whether it's best included.
get_unused_fd_flags() doesn't take other flags than O_CLOEXEC, we need
to set it directly like we already do for f_mode.
On 64bit O_LARGEFILE is already forced for many syscalls. I added it
now as it makes perfect sense. It's part of the memfd ABI now.
man-page is fixed, too.
Thanks
David
>> + if (fd < 0) {
>> + error = fd;
>> + goto err_name;
>> + }
>> +
>> + file = shmem_file_setup(name, 0, VM_NORESERVE);
>> + if (IS_ERR(file)) {
>> + error = PTR_ERR(file);
>> + goto err_fd;
>> + }
>> + info = SHMEM_I(file_inode(file));
>> + file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
>> + if (flags & MFD_ALLOW_SEALING)
>> + info->seals &= ~F_SEAL_SEAL;
>> +
>> + fd_install(fd, file);
>> + kfree(name);
>> + return fd;
>> +
>> +err_fd:
>> + put_unused_fd(fd);
>> +err_name:
>> + kfree(name);
>> + return error;
>> +}
>> +
>> #endif /* CONFIG_TMPFS */
>>
>> static void shmem_put_super(struct super_block *sb)
>> --
>> 2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists