* Re: [PATCH v3 1/7] mm, slab: combine kmalloc_caches and kmalloc_dma_caches
From: Christopher Lameter @ 2018-07-30 15:38 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-2-vbabka@suse.cz>
On Wed, 18 Jul 2018, Vlastimil Babka wrote:
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -295,12 +295,28 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
> #define SLAB_OBJ_MIN_SIZE (KMALLOC_MIN_SIZE < 16 ? \
> (KMALLOC_MIN_SIZE) : 16)
>
> +#define KMALLOC_NORMAL 0
> +#ifdef CONFIG_ZONE_DMA
> +#define KMALLOC_DMA 1
> +#define KMALLOC_TYPES 2
> +#else
> +#define KMALLOC_TYPES 1
> +#endif
An emum would be better here I think.
But the patch is ok
Acked-by: Christoph Lameter <cl@linux.com>
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-30 14:48 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, Linux FS Devel,
LKML
In-Reply-To: <CALCETrW6HQnGnnmzNrNAeEyRZZv9uJFcH92jjofR1Rx5NQTxDQ@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> wrote:
> Constants are almost always all caps. Is there any reason these are
> lowercase?
It's also easier to create macros that pair structs or functions with the
contants by name.
David
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: David Howells @ 2018-07-30 12:32 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, linux-fsdevel,
kernel list
In-Reply-To: <CAG48ez3HY6UuOgo1gM9L=WwyOqifKSs6sFu7f2qqBZQg6hy96g@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > > This means that a namespace admin (iow, an unprivileged user) can
> > > allocate 1MB of unswappable kmalloc memory per userspace task, right?
> > > Using userfaultfd or FUSE, you can then stall the task as long as you
> > > want while it has that allocation. Is that problematic, or is that
> > > normal?
> >
> > That's not exactly the case. A userspace task can make a temporary
> > allocation, but unless the filesystem grabs it, it's released again on exit
> > from the system call.
>
> That's what I said.
Sorry, I wasn't clear what you meant. I assumed you were thinking it was then
automatically attached to the context, say:
fd = fsopen("fuse", 0);
fsconfig(fd, fsconfig_set_binary, "foo", buffer, size);
> Each userspace task can make a 1MB allocation by calling this syscall, and
> this temporary allocation stays allocated until the end of the syscall. But
> the runtime of the syscall is unbounded - even just the memdup_user_nul()
> can stall forever if the copy_from_user() call inside it faults on e.g. a
> userfault region or a memory-mapped file from a FUSE filesystem.
Okay, I see what you're getting at. Note that this affects other syscalls
too, keyctl, module loading and read() with readahead for example. Not sure
what the answer should be.
David
^ permalink raw reply
* Re: [PATCH] prctl: add PR_[GS]ET_KILLABLE
From: Oleg Nesterov @ 2018-07-30 10:17 UTC (permalink / raw)
To: Jürg Billeter; +Cc: Andrew Morton, Eric Biederman, linux-api, linux-kernel
In-Reply-To: <20180730075241.24002-1-j@bitron.ch>
On 07/30, Jürg Billeter wrote:
>
> This is required for job control in a shell that uses CLONE_NEWPID for
> child processes.
Could you explain in more details?
> + case PR_SET_KILLABLE:
> + if (arg2 != 1 || arg3 || arg4 || arg5)
> + return -EINVAL;
> + me->signal->flags &= ~SIGNAL_UNKILLABLE;
this needs spin_lock_irq(me->sighand->siglock).
Oleg.
^ permalink raw reply
* Re: [PATCH 4/4] mm: proc/pid/smaps_rollup: convert to single value seq_file
From: Vlastimil Babka @ 2018-07-30 8:53 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Andrew Morton, Daniel Colascione, linux-mm, linux-kernel,
linux-fsdevel, linux-api
In-Reply-To: <20180726162637.GB25227@avx2>
On 07/26/2018 06:26 PM, Alexey Dobriyan wrote:
> On Wed, Jul 25, 2018 at 08:53:53AM +0200, Vlastimil Babka wrote:
>> I moved the reply to this thread since the "added to -mm tree"
>> notification Alexey replied to in <20180724182908.GD27053@avx2> has
>> reduced CC list and is not linked to the patch postings.
>>
>> On 07/24/2018 08:29 PM, Alexey Dobriyan wrote:
>>> On Mon, Jul 23, 2018 at 04:55:48PM -0700, akpm@linux-foundation.org wrote:
>>>> The patch titled
>>>> Subject: mm: /proc/pid/smaps_rollup: convert to single value seq_file
>>>> has been added to the -mm tree. Its filename is
>>>> mm-proc-pid-smaps_rollup-convert-to-single-value-seq_file.patch
>>>
>>>> Subject: mm: /proc/pid/smaps_rollup: convert to single value seq_file
>>>>
>>>> The /proc/pid/smaps_rollup file is currently implemented via the
>>>> m_start/m_next/m_stop seq_file iterators shared with the other maps files,
>>>> that iterate over vma's. However, the rollup file doesn't print anything
>>>> for each vma, only accumulate the stats.
>>>
>>> What I don't understand why keep seq_ops then and not do all the work in
>>> ->show hook. Currently /proc/*/smaps_rollup is at ~500 bytes so with
>>> minimum 1 page seq buffer, no buffer resizing is possible.
>>
>> Hmm IIUC seq_file also provides the buffer and handles feeding the data
>> from there to the user process, which might have called read() with a smaller
>> buffer than that. So I would rather not avoid the seq_file infrastructure.
>> Or you're saying it could be converted to single_open()? Maybe, with more work.
>
> Prefereably yes.
OK here it is. Sending as a new patch instead of delta, as that's easier
to review - the delta is significant. Line stats wise it's the same.
Again a bit less boilerplate thans to no special seq_ops, a bit more
copy/paste in the open and release functions. But I guess it's better
overall.
----8>----
>From c6a2eaf3bb3546509d6b7c42f8bcc56cd7e92f90 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 18 Jul 2018 13:14:30 +0200
Subject: [PATCH] mm: proc/pid/smaps_rollup: convert to single value seq_file
The /proc/pid/smaps_rollup file is currently implemented via the
m_start/m_next/m_stop seq_file iterators shared with the other maps files,
that iterate over vma's. However, the rollup file doesn't print anything
for each vma, only accumulate the stats.
There are some issues with the current code as reported in [1] - the
accumulated stats can get skewed if seq_file start()/stop() op is called
multiple times, if show() is called multiple times, and after seeks to
non-zero position.
Patch [1] fixed those within existing design, but I believe it is
fundamentally wrong to expose the vma iterators to the seq_file mechanism
when smaps_rollup shows logically a single set of values for the whole
address space.
This patch thus refactors the code to provide a single "value" at offset 0,
with vma iteration to gather the stats done internally. This fixes the
situations where results are skewed, and simplifies the code, especially
in show_smap(), at the expense of somewhat less code reuse.
[1] https://marc.info/?l=linux-mm&m=151927723128134&w=2
Reported-by: Daniel Colascione <dancol@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
fs/proc/internal.h | 1 -
fs/proc/task_mmu.c | 155 ++++++++++++++++++++++++++++-----------------
2 files changed, 96 insertions(+), 60 deletions(-)
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 0c538769512a..f5b75d258d22 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -285,7 +285,6 @@ struct proc_maps_private {
struct inode *inode;
struct task_struct *task;
struct mm_struct *mm;
- struct mem_size_stats *rollup;
#ifdef CONFIG_MMU
struct vm_area_struct *tail_vma;
#endif
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index c47f3cab70a1..5ea1d64cb0b4 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -247,7 +247,6 @@ static int proc_map_release(struct inode *inode, struct file *file)
if (priv->mm)
mmdrop(priv->mm);
- kfree(priv->rollup);
return seq_release_private(inode, file);
}
@@ -404,7 +403,6 @@ const struct file_operations proc_pid_maps_operations = {
#ifdef CONFIG_PROC_PAGE_MONITOR
struct mem_size_stats {
- bool first;
unsigned long resident;
unsigned long shared_clean;
unsigned long shared_dirty;
@@ -418,7 +416,6 @@ struct mem_size_stats {
unsigned long swap;
unsigned long shared_hugetlb;
unsigned long private_hugetlb;
- unsigned long first_vma_start;
u64 pss;
u64 pss_locked;
u64 swap_pss;
@@ -775,57 +772,75 @@ static void __show_smap(struct seq_file *m, const struct mem_size_stats *mss)
static int show_smap(struct seq_file *m, void *v)
{
- struct proc_maps_private *priv = m->private;
struct vm_area_struct *vma = v;
- struct mem_size_stats mss_stack;
- struct mem_size_stats *mss;
+ struct mem_size_stats mss;
+
+ memset(&mss, 0, sizeof(mss));
+
+ smap_gather_stats(vma, &mss);
+
+ show_map_vma(m, vma);
+
+ SEQ_PUT_DEC("Size: ", vma->vm_end - vma->vm_start);
+ SEQ_PUT_DEC(" kB\nKernelPageSize: ", vma_kernel_pagesize(vma));
+ SEQ_PUT_DEC(" kB\nMMUPageSize: ", vma_mmu_pagesize(vma));
+ seq_puts(m, " kB\n");
+
+ __show_smap(m, &mss);
+
+ if (arch_pkeys_enabled())
+ seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
+ show_smap_vma_flags(m, vma);
+
+ m_cache_vma(m, vma);
+
+ return 0;
+}
+
+static int show_smaps_rollup(struct seq_file *m, void *v)
+{
+ struct proc_maps_private *priv = m->private;
+ struct mem_size_stats mss;
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ unsigned long last_vma_end = 0;
int ret = 0;
- bool rollup_mode;
- bool last_vma;
-
- if (priv->rollup) {
- rollup_mode = true;
- mss = priv->rollup;
- if (mss->first) {
- mss->first_vma_start = vma->vm_start;
- mss->first = false;
- }
- last_vma = !m_next_vma(priv, vma);
- } else {
- rollup_mode = false;
- memset(&mss_stack, 0, sizeof(mss_stack));
- mss = &mss_stack;
- }
- smap_gather_stats(vma, mss);
+ priv->task = get_proc_task(priv->inode);
+ if (!priv->task)
+ return -ESRCH;
- if (!rollup_mode) {
- show_map_vma(m, vma);
- } else if (last_vma) {
- show_vma_header_prefix(
- m, mss->first_vma_start, vma->vm_end, 0, 0, 0, 0);
- seq_pad(m, ' ');
- seq_puts(m, "[rollup]\n");
- } else {
- ret = SEQ_SKIP;
+ mm = priv->mm;
+ if (!mm || !mmget_not_zero(mm)) {
+ ret = -ESRCH;
+ goto out_put_task;
}
- if (!rollup_mode) {
- SEQ_PUT_DEC("Size: ", vma->vm_end - vma->vm_start);
- SEQ_PUT_DEC(" kB\nKernelPageSize: ", vma_kernel_pagesize(vma));
- SEQ_PUT_DEC(" kB\nMMUPageSize: ", vma_mmu_pagesize(vma));
- seq_puts(m, " kB\n");
- }
+ memset(&mss, 0, sizeof(mss));
- if (!rollup_mode || last_vma)
- __show_smap(m, mss);
+ down_read(&mm->mmap_sem);
+ hold_task_mempolicy(priv);
- if (!rollup_mode) {
- if (arch_pkeys_enabled())
- seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
- show_smap_vma_flags(m, vma);
+ for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
+ smap_gather_stats(vma, &mss);
+ last_vma_end = vma->vm_end;
}
- m_cache_vma(m, vma);
+
+ show_vma_header_prefix(m, priv->mm->mmap->vm_start,
+ last_vma_end, 0, 0, 0, 0);
+ seq_pad(m, ' ');
+ seq_puts(m, "[rollup]\n");
+
+ __show_smap(m, &mss);
+
+ release_task_mempolicy(priv);
+ up_read(&mm->mmap_sem);
+ mmput(mm);
+
+out_put_task:
+ put_task_struct(priv->task);
+ priv->task = NULL;
+
return ret;
}
#undef SEQ_PUT_DEC
@@ -842,23 +857,45 @@ static int pid_smaps_open(struct inode *inode, struct file *file)
return do_maps_open(inode, file, &proc_pid_smaps_op);
}
-static int pid_smaps_rollup_open(struct inode *inode, struct file *file)
+static int smaps_rollup_open(struct inode *inode, struct file *file)
{
- struct seq_file *seq;
+ int ret;
struct proc_maps_private *priv;
- int ret = do_maps_open(inode, file, &proc_pid_smaps_op);
-
- if (ret < 0)
- return ret;
- seq = file->private_data;
- priv = seq->private;
- priv->rollup = kzalloc(sizeof(*priv->rollup), GFP_KERNEL);
- if (!priv->rollup) {
- proc_map_release(inode, file);
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL_ACCOUNT);
+ if (!priv)
return -ENOMEM;
+
+ ret = single_open(file, show_smaps_rollup, priv);
+ if (ret)
+ goto out_free;
+
+ priv->inode = inode;
+ priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
+ if (IS_ERR(priv->mm)) {
+ ret = PTR_ERR(priv->mm);
+
+ single_release(inode, file);
+ goto out_free;
}
- priv->rollup->first = true;
+
return 0;
+
+out_free:
+ kfree(priv);
+ return ret;
+}
+
+static int smaps_rollup_release(struct inode *inode, struct file *file)
+{
+ struct seq_file *seq = file->private_data;
+ struct proc_maps_private *priv = seq->private;
+
+ if (priv->mm)
+ mmdrop(priv->mm);
+
+ kfree(priv);
+ return single_release(inode, file);
}
const struct file_operations proc_pid_smaps_operations = {
@@ -869,10 +906,10 @@ const struct file_operations proc_pid_smaps_operations = {
};
const struct file_operations proc_pid_smaps_rollup_operations = {
- .open = pid_smaps_rollup_open,
+ .open = smaps_rollup_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = proc_map_release,
+ .release = smaps_rollup_release,
};
enum clear_refs_types {
--
2.18.0
^ permalink raw reply related
* Re: [PATCH 2/4] mm: proc/pid/smaps: factor out mem stats gathering
From: Vlastimil Babka @ 2018-07-30 8:45 UTC (permalink / raw)
To: Andrew Morton
Cc: Daniel Colascione, linux-mm, linux-kernel, linux-fsdevel,
Alexey Dobriyan, linux-api
In-Reply-To: <20180723111933.15443-3-vbabka@suse.cz>
(moved thread here)
On 07/26/2018 06:21 PM, Alexey Dobriyan wrote:
> On Wed, Jul 25, 2018 at 08:55:17AM +0200, Vlastimil Babka wrote:
>> On 07/24/2018 08:24 PM, Alexey Dobriyan wrote:
>>> On Mon, Jul 23, 2018 at 04:55:46PM -0700, akpm@linux-foundation.org wrote:
>>>> The patch titled
>>>> Subject: mm: /proc/pid/smaps: factor out common stats printing
>>>> has been added to the -mm tree. Its filename is
>>>> mm-proc-pid-smaps-factor-out-common-stats-printing.patch
>>>> +/* Show the contents common for smaps and smaps_rollup */
>>>> +static void __show_smap(struct seq_file *m, struct mem_size_stats *mss)
>>> This can be "const".
>> What exactly, mss?
> Yes, of course.
> seq_file is changed by virtue of priting to it.
----8<----
>From 71a7c912496db1847e3b265cd30922c4e687b7c2 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Fri, 27 Jul 2018 13:56:00 +0200
Subject: [PATCH] mm: proc/pid/smaps: factor out common stats printing-fix
Add const, per Alexey.
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
fs/proc/task_mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 1d6d315fd31b..c47f3cab70a1 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -749,7 +749,7 @@ static void smap_gather_stats(struct vm_area_struct *vma,
seq_put_decimal_ull_width(m, str, (val) >> 10, 8)
/* Show the contents common for smaps and smaps_rollup */
-static void __show_smap(struct seq_file *m, struct mem_size_stats *mss)
+static void __show_smap(struct seq_file *m, const struct mem_size_stats *mss)
{
SEQ_PUT_DEC("Rss: ", mss->resident);
SEQ_PUT_DEC(" kB\nPss: ", mss->pss >> PSS_SHIFT);
--
2.18.0
^ permalink raw reply related
* [PATCH] prctl: add PR_[GS]ET_KILLABLE
From: Jürg Billeter @ 2018-07-30 7:52 UTC (permalink / raw)
To: Andrew Morton
Cc: Oleg Nesterov, Eric Biederman, linux-api, linux-kernel,
Jürg Billeter
PR_SET_KILLABLE clears the SIGNAL_UNKILLABLE flag. This allows
CLONE_NEWPID tasks to restore normal signal behavior, opting out of the
special signal protection for init processes.
This is required for job control in a shell that uses CLONE_NEWPID for
child processes.
This prctl does not allow setting the SIGNAL_UNKILLABLE flag, only
clearing.
Signed-off-by: Jürg Billeter <j@bitron.ch>
---
include/uapi/linux/prctl.h | 4 ++++
kernel/sys.c | 11 +++++++++++
2 files changed, 15 insertions(+)
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index c0d7ea0bf5b6..92afb63da727 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -219,4 +219,8 @@ struct prctl_mm_map {
# define PR_SPEC_DISABLE (1UL << 2)
# define PR_SPEC_FORCE_DISABLE (1UL << 3)
+/* Control SIGNAL_UNKILLABLE */
+#define PR_GET_KILLABLE 54
+#define PR_SET_KILLABLE 55
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..264de630d548 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2484,6 +2484,17 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
error = arch_prctl_spec_ctrl_set(me, arg2, arg3);
break;
+ case PR_GET_KILLABLE:
+ if (arg3 || arg4 || arg5)
+ return -EINVAL;
+ error = put_user(!(me->signal->flags & SIGNAL_UNKILLABLE),
+ (int __user *)arg2);
+ break;
+ case PR_SET_KILLABLE:
+ if (arg2 != 1 || arg3 || arg4 || arg5)
+ return -EINVAL;
+ me->signal->flags &= ~SIGNAL_UNKILLABLE;
+ break;
default:
error = -EINVAL;
break;
--
2.18.0
^ permalink raw reply related
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: Jann Horn @ 2018-07-29 11:14 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Linux API, Linus Torvalds, linux-fsdevel, kernel list
In-Reply-To: <19865.1532854200@warthog.procyon.org.uk>
On Sun, Jul 29, 2018 at 10:50 AM David Howells <dhowells@redhat.com> wrote:
>
> Jann Horn <jannh@google.com> wrote:
>
> > [...]
> > > + case fsconfig_set_binary:
> > > + param.type = fs_value_is_blob;
> > > + param.size = aux;
> > > + param.blob = memdup_user_nul(_value, aux);
> > > + if (IS_ERR(param.blob)) {
> > > + ret = PTR_ERR(param.blob);
> > > + goto out_key;
> > > + }
> > > + break;
> >
> > This means that a namespace admin (iow, an unprivileged user) can
> > allocate 1MB of unswappable kmalloc memory per userspace task, right?
> > Using userfaultfd or FUSE, you can then stall the task as long as you
> > want while it has that allocation. Is that problematic, or is that
> > normal?
>
> That's not exactly the case. A userspace task can make a temporary
> allocation, but unless the filesystem grabs it, it's released again on exit
> from the system call.
That's what I said. Each userspace task can make a 1MB allocation by
calling this syscall, and this temporary allocation stays allocated
until the end of the syscall. But the runtime of the syscall is
unbounded - even just the memdup_user_nul() can stall forever if the
copy_from_user() call inside it faults on e.g. a userfault region or a
memory-mapped file from a FUSE filesystem.
> Note that I should probably use vmalloc() rather than kmalloc(), but that
> doesn't really affect your point. I could also pass the user pointer through
> to the filesystem instead - I wanted to avoid that for this interface, but it
> make sense in this instance.
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: David Howells @ 2018-07-29 8:50 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, linux-fsdevel,
kernel list
In-Reply-To: <CAG48ez2f+J+UeFN0u1Hpfq3tFXJbi7ckts8sua3eY4jDwnh6-Q@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> [...]
> > + case fsconfig_set_binary:
> > + param.type = fs_value_is_blob;
> > + param.size = aux;
> > + param.blob = memdup_user_nul(_value, aux);
> > + if (IS_ERR(param.blob)) {
> > + ret = PTR_ERR(param.blob);
> > + goto out_key;
> > + }
> > + break;
>
> This means that a namespace admin (iow, an unprivileged user) can
> allocate 1MB of unswappable kmalloc memory per userspace task, right?
> Using userfaultfd or FUSE, you can then stall the task as long as you
> want while it has that allocation. Is that problematic, or is that
> normal?
That's not exactly the case. A userspace task can make a temporary
allocation, but unless the filesystem grabs it, it's released again on exit
from the system call.
Note that I should probably use vmalloc() rather than kmalloc(), but that
doesn't really affect your point. I could also pass the user pointer through
to the filesystem instead - I wanted to avoid that for this interface, but it
make sense in this instance.
David
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Pavel Machek @ 2018-07-28 14:13 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Carlos O'Donell, Florian Weimer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett
In-Reply-To: <1210024721.6363.1532785744879.JavaMail.zimbra@efficios.com>
[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]
Hi!
> Documentation-wise, I have posted a rseq man page rfc here:
>
> https://lkml.kernel.org/r/20180616195803.29877-1-mathieu.desnoyers@efficios.com
>
> comments are welcome!
Thanks for pointer.
+Restartable sequences are atomic with respect to preemption (making
it
+atomic with respect to other threads running on the same CPU), as
well
+as signal delivery (user-space execution contexts nested over the
same
+thread).
So the threads are protected against sigkill when running the
restartable sequence?
+Restartable sequences must not perform system calls. Doing so may
result
+in termination of the process by a segmentation fault.
+
"may result"? It would be nice to always catch that.
+Optimistic cache of the CPU number on which the current thread is
+running. Its value is guaranteed to always be a possible CPU number,
+even when rseq is not initialized. The value it contains should
always
+be confirmed by reading the cpu_id field.
I'm not sure what "optimistic cache" is...
+Flags indicating the restart behavior for the current thread. This is
+mainly used for debugging purposes. Can be either:
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
+.IP \[bu]
Flags tell me there may be more then one, but "can be either" tells me
just one flag is allowed.
+.B Structure alignment
+This structure is aligned on multiples of 32 bytes.
+.TP
+.B Structure size
+This structure has a fixed size of 32 bytes.
+.B Structure alignment
+This structure is aligned on multiples of 32 bytes.
+.TP
+.B Structure size
+This structure has a fixed size of 32 bytes.
I believe we normally say "is aligned on 32-bytes boundary". (Will not
this need to be bigger on machines with bigger cache sizes?)
above it says:
+.B Structure size
+This structure is extensible. Its size is passed as parameter to the
+rseq system call.
I'm reading source, so maybe it refers to different structure.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Mathieu Desnoyers @ 2018-07-28 13:49 UTC (permalink / raw)
To: Pavel Machek, Carlos O'Donell, Florian Weimer
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <20180727220115.GA18879@amd>
----- On Jul 27, 2018, at 6:01 PM, Pavel Machek pavel@ucw.cz wrote:
> Hi!
>
>> So for instance, this turns:
>>
>> int cpu = rseq_per_cpu_lock(lock, target_cpu);
>> [...]
>> rseq_per_cpu_unlock(lock, cpu);
>>
>> into
>>
>> int cpu = rseq_this_cpu_lock(lock);
>> [...]
>> rseq_per_cpu_unlock(lock, cpu);
>>
>> and:
>>
>> per_cpu_list_push(list, node, target_cpu);
>> [...]
>> per_cpu_list_pop(list, node, target_cpu);
>>
>> into
>>
>> this_cpu_list_push(list, node, &cpu); /* cpu is an output parameter. */
>> [...]
>> node = this_cpu_list_pop(list, &cpu); /* cpu is an output parameter. */
>>
>> Eventually integrating cpu_opv or some alternative will allow passing
>> the cpu number as parameter rather than requiring the algorithm to work
>> on the current CPU.
>>
>> The second effect of not having the cpu_opv fallback is that
>> line and instruction single-stepping with a debugger transforms rseq
>> critical sections based on retry loops into never-ending loops.
>> Debuggers need to use the __rseq_table section to skip those critical
>> sections in order to correctly behave when single-stepping a thread
>> which uses rseq in a retry loop. However, applications which use an
>> alternative fallback method rather than retrying on rseq fast-path abort
>> won't be affected by this kind of single-stepping issue.
>>
>> Thanks for your feedback!
>
> Would it make sense to include Documentation/ patch? I guess at least
> manpage describing the syscall will be needed....
Hi Pavel,
Documentation-wise, I have posted a rseq man page rfc here:
https://lkml.kernel.org/r/20180616195803.29877-1-mathieu.desnoyers@efficios.com
comments are welcome!
It does not include any details about user-space library APIs though, as
this is not the purpose of the syscall documentation.
We're currently discussing integration of rseq thread registration into
glibc.
Once this is settled, I plan to provide a librseq which will contain headers
and documentation on how to use rseq without having to re-create the low-level
assembly every time.
Does this plan make sense to you ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: Anton Altaparmakov @ 2018-07-28 0:14 UTC (permalink / raw)
To: David Howells
Cc: Jann Horn, Al Viro, Linux API, Linus Torvalds,
linux-fsdevel@vger.kernel.org, kernel list
In-Reply-To: <21395.1532735340@warthog.procyon.org.uk>
Hi David,
> On 28 Jul 2018, at 00:49, David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
>>> +static int fsinfo_generic_name_encoding(struct dentry *dentry, char *buf)
>>> +{
>>> + static const char encoding[] = "utf8";
>>> +
>>> + if (buf)
>>> + memcpy(buf, encoding, sizeof(encoding) - 1);
>>> + return sizeof(encoding) - 1;
>>> +}
>>
>> Is this meant to be "encoding to be used by userspace" or "encoding of
>> on-disk filenames"?
>
> The latter.
>
>> Are there any plans to create filesystems that behave differently?
>
> isofs, fat, ntfs, cifs for example.
>
>> If the latter: This is wrong for e.g. a vfat mount that uses a codepage,
>> right? Should the default in that case not be "I don't know"?
>
> Quite possibly. Note that it could also be what you're interpreting it as
> because the codepage got overridden by a mount parameter rather than what's on
> the disk (assuming the medium actually records this).
No, nothing like that is recorded on disk. That would have been way too helpful! (-; The only place Windows records such information is, you may have guessed this: in the registry which of course is local to the computer and unrelated to what removable media is attached...
> One thing I'm confused about is that fat has both a codepage and a charset and
> I'm not sure of the difference.
Oh that is quite simple. (-:
The codepage is what is used to translate from/to the on-disk DOS 8.3 style names into the kernel's Unicode character representation. The correct codepage for a particular volume is not stored on disk so it can lead to all sorts of fun if you for example create some names on for example a Japanese Windows on a FAT formatted USB stick and then plug that into a US or European Windows where the default code pages are completely different - all your filenames will appear totally corrupt. (Note this ONLY affects 8.3 style/DOS/short names or whatever you want to call them.)
The charset on the other hand is what is used to convert strings coming in from/going out to userspace into the kernel's Unicode character representation.
The one nice thing about VFAT (and there aren't many nice things about it!) is that for long names (i.e. not the 8.3 style/DOS/short names), it actually stores on-disk little-endian UTF-16 (since Windows 2000, before that it used little endian UCS-2 - the change was needed to support things like Emojis and some languages that go outside the UCS-2 range of fixed 16-bit unicode).
Hope this clears that up.
Best regards,
Anton
> David
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-28 0:08 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, linux-fsdevel,
kernel list
In-Reply-To: <CAG48ez3vVEEbJzbK8Nc6HA-W7Sp7==ZXanzFUKo527p4U=OFpA@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > fs/fat/inode.c: sprintf(buf, "cp%d", sbi->options.codepage);
>
> sprintf(buf, "cp%d", sbi->options.codepage);
> sbi->nls_disk = load_nls(buf);
> if (!sbi->nls_disk) {
> fat_msg(sb, KERN_ERR, "codepage %s not found", buf);
> goto out_fail;
> }
Sorry, yes. I was reading the print as the part of the display of superblock
parameters.
David
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: Jann Horn @ 2018-07-27 23:58 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Linux API, Linus Torvalds, linux-fsdevel, kernel list
In-Reply-To: <21746.1532735504@warthog.procyon.org.uk>
On Sat, Jul 28, 2018 at 1:51 AM David Howells <dhowells@redhat.com> wrote:
> David Howells <dhowells@redhat.com> wrote:
>
> > One thing I'm confused about is that fat has both a codepage and a charset and
> > I'm not sure of the difference.
>
> In fact, it's not clear that the codepage is actually used.
>
> warthog>git grep '[.>]codepage'
> fs/fat/inode.c: opts->codepage = fat_default_codepage;
> fs/fat/inode.c: opts->codepage = option;
> fs/fat/inode.c: sprintf(buf, "cp%d", sbi->options.codepage);
sprintf(buf, "cp%d", sbi->options.codepage);
sbi->nls_disk = load_nls(buf);
if (!sbi->nls_disk) {
fat_msg(sb, KERN_ERR, "codepage %s not found", buf);
goto out_fail;
}
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-27 23:51 UTC (permalink / raw)
Cc: dhowells, Jann Horn, Al Viro, Linux API, Linus Torvalds,
linux-fsdevel, kernel list
In-Reply-To: <21395.1532735340@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> wrote:
> One thing I'm confused about is that fat has both a codepage and a charset and
> I'm not sure of the difference.
In fact, it's not clear that the codepage is actually used.
warthog>git grep '[.>]codepage'
fs/fat/inode.c: opts->codepage = fat_default_codepage;
fs/fat/inode.c: opts->codepage = option;
fs/fat/inode.c: sprintf(buf, "cp%d", sbi->options.codepage);
David
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-27 23:49 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, linux-fsdevel,
kernel list
In-Reply-To: <CAG48ez0SdSPkimsk-MW7oSQO-Y64X-LD4JXnuPsZXKuz=y1_Kg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > +static int fsinfo_generic_name_encoding(struct dentry *dentry, char *buf)
> > +{
> > + static const char encoding[] = "utf8";
> > +
> > + if (buf)
> > + memcpy(buf, encoding, sizeof(encoding) - 1);
> > + return sizeof(encoding) - 1;
> > +}
>
> Is this meant to be "encoding to be used by userspace" or "encoding of
> on-disk filenames"?
The latter.
> Are there any plans to create filesystems that behave differently?
isofs, fat, ntfs, cifs for example.
> If the latter: This is wrong for e.g. a vfat mount that uses a codepage,
> right? Should the default in that case not be "I don't know"?
Quite possibly. Note that it could also be what you're interpreting it as
because the codepage got overridden by a mount parameter rather than what's on
the disk (assuming the medium actually records this).
One thing I'm confused about is that fat has both a codepage and a charset and
I'm not sure of the difference.
David
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: Jann Horn @ 2018-07-27 23:14 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Linux API, Linus Torvalds, linux-fsdevel, kernel list
In-Reply-To: <153271291017.9458.7827028432894772673.stgit@warthog.procyon.org.uk>
On Fri, Jul 27, 2018 at 7:36 PM David Howells <dhowells@redhat.com> wrote:
>
> Add a system call to allow filesystem information to be queried. A request
> value can be given to indicate the desired attribute. Support is provided
> for enumerating multi-value attributes.
[...]
> +static int fsinfo_generic_ids(struct dentry *dentry,
> + struct fsinfo_ids *p)
> +{
[...]
> + strcpy(p->f_fs_name, dentry->d_sb->s_type->name);
Can you use strlcpy() instead? From a quick look, I don't see anything
that actually limits the size of filesystem names, even though
everything in-kernel probably fits into the 16 bytes you've allocated
for the name.
[...]
> +static int fsinfo_generic_name_encoding(struct dentry *dentry, char *buf)
> +{
> + static const char encoding[] = "utf8";
> +
> + if (buf)
> + memcpy(buf, encoding, sizeof(encoding) - 1);
> + return sizeof(encoding) - 1;
> +}
Is this meant to be "encoding to be used by userspace" or "encoding of
on-disk filenames"? If the former: That's always utf8, right? Are
there any plans to create filesystems that behave differently? If the
latter: This is wrong for e.g. a vfat mount that uses a codepage,
right? Should the default in that case not be "I don't know"?
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: Jann Horn @ 2018-07-27 22:32 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Linux API, Linus Torvalds, linux-fsdevel, kernel list
In-Reply-To: <153271287586.9458.6001928723332685410.stgit@warthog.procyon.org.uk>
On Fri, Jul 27, 2018 at 7:34 PM David Howells <dhowells@redhat.com> wrote:
>
> Add a syscall for configuring a filesystem creation context and triggering
> actions upon it, to be used in conjunction with fsopen, fspick and fsmount.
>
> long fsconfig(int fs_fd, unsigned int cmd, const char *key,
> const void *value, int aux);
>
> Where fs_fd indicates the context, cmd indicates the action to take, key
> indicates the parameter name for parameter-setting actions and, if needed,
> value points to a buffer containing the value and aux can give more
> information for the value.
[...]
> +SYSCALL_DEFINE5(fsconfig,
> + int, fd,
> + unsigned int, cmd,
> + const char __user *, _key,
> + const void __user *, _value,
> + int, aux)
> +{
[...]
> + switch (cmd) {
[...]
> + case fsconfig_set_binary:
> + if (!_key || !_value || aux <= 0 || aux > 1024 * 1024)
> + return -EINVAL;
> + break;
[...]
> + }
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> + ret = -EINVAL;
> + if (f.file->f_op != &fscontext_fops)
> + goto out_f;
We should probably add an fdget_typed(fd, fops) helper, or something
like that, to file.h at some point... there are probably dozens of
such invocations across the kernel at this point, each one with a
couple lines of boilerplate to deal with the two separate error paths.
[...]
> + case fsconfig_set_binary:
> + param.type = fs_value_is_blob;
> + param.size = aux;
> + param.blob = memdup_user_nul(_value, aux);
> + if (IS_ERR(param.blob)) {
> + ret = PTR_ERR(param.blob);
> + goto out_key;
> + }
> + break;
This means that a namespace admin (iow, an unprivileged user) can
allocate 1MB of unswappable kmalloc memory per userspace task, right?
Using userfaultfd or FUSE, you can then stall the task as long as you
want while it has that allocation. Is that problematic, or is that
normal?
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: David Howells @ 2018-07-27 22:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, Linux FS Devel,
LKML
In-Reply-To: <CALCETrWDGa+ut=xsoki+XFKzdD7KruJJicfH1tKdBtywLuk7iQ@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> wrote:
> > [*] Not without a 6-arg syscall or some other way of passing it.
>
> Are there still architectures that have problems with 6-arg syscalls?
As I understand it, 6-arg syscalls are frowned upon.
> I suppose that, as long as there is never a case where fsconfig_set_path and
> fsconfig_set_fd both succeed, then it's not a big deal.
fsconfig_set_path/path_empty requires the 'value' argument to point to a
string, possibly "", and fsconfig_set_fd requires it to be NULL.
I can't stop you from doing:
fd = open("/some/path", O_PATH);
fsconfig(fsfd, fsconfig_set_fd, "fd", NULL, fd);
or:
fd = open("/dev/sda6", O_RDWR);
fsconfig(fsfd, fsconfig_set_path_empty, "foo", "", fd);
The first should fail because I'm using fget() not fget_raw() and the
second will pass the string and fd number to the filesystem, which will
presumably then call fs_lookup_param() to invoke pathwalk upon it - which will
likely also fail.
David
^ permalink raw reply
* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-27 22:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, Linux FS Devel,
LKML
In-Reply-To: <CALCETrW6HQnGnnmzNrNAeEyRZZv9uJFcH92jjofR1Rx5NQTxDQ@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> wrote:
> > Add a system call to allow filesystem information to be queried. A request
> > value can be given to indicate the desired attribute. Support is provided
> > for enumerating multi-value attributes.
>
> Has anyone seriously reviewed this?
I don't know. I've certainly posted it before.
> > params->request indicates the attribute/attributes to be queried. This can
> > be one of:
> >
> > fsinfo_attr_statfs - statfs-style info
> > fsinfo_attr_fsinfo - Information about fsinfo()
>
> Constants are almost always all caps. Is there any reason these are
> lowercase?
It looks better IMO, particularly for enum constants. I'm not sure if there
are any rules about this in system vs user definitions.
David
^ permalink raw reply
* Re: [PATCH 30/38] vfs: syscall: Add fsmount() to create a mount for a superblock [ver #10]
From: David Howells @ 2018-07-27 22:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, Linux FS Devel,
LKML
In-Reply-To: <CALCETrVmZKeFv7b+c8hBQ-3Dcryv7VXTHdyWftvNG1G4aQk9Mw@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> wrote:
> int mfd = fsmount(...);
>
> where you pass in an fscontext fd and get out an fd referring to the
> root of the filesystem? In this case, maybe fs_open_root(2) would be
> a better name.
It's not necessarily the root of the filesystem in the sense of sb->s_root.
It might be a subset of that, or it might be a part of a filesystem that might
have multiple roots because it doesn't know where the real root is (NFS2, for
example).
> This *definitely* needs to be clearer in the description.
I'm open to suggestions of better wording. It's a bit hard to explain
because, as you pointed out, the terminology is overloaded.
David
^ permalink raw reply
* Re: [PATCH 30/38] vfs: syscall: Add fsmount() to create a mount for a superblock [ver #10]
From: David Howells @ 2018-07-27 22:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, viro, linux-api, torvalds, linux-fsdevel, linux-kernel
In-Reply-To: <D09916DC-DA22-48AA-A62B-F874BB3C15BC@amacapital.net>
Andy Lutomirski <luto@amacapital.net> wrote:
> I have a potentially silly objection. For the old timers, "mount" means to
> stick a reel of tape or some similar object onto a reader, which seems to
> imply that "mount" means to start up the filesystem. For younguns, this
> meaning is probably lost, and the more obvious meaning is to "mount" it into
> some location in the VFS hierarchy a la vfsmount. The patch description
> doesn't disambiguate it, and obviously people used to mount(2)/mount(8) are
> just likely to be confused.
The problem is that inside the kernel it *is* a "mount".
How about I change the first paragraph to:
Provide a system call by which a filesystem opened with fsopen() and
configured by a series of fsconfig() calls can have a detached mount
object created for it. This mount object can then be attached to the
VFS mount hierarchy using move_mount() by passing the returned file
descriptor as the from directory fd.
> At the very least, your description should make it absolutely clear what you
> mean. Even better IMO would be to drop the use of the word "mount" entirely
I'm not sure that's a reasonable idea, given the "mounting" is how this is
done.
Can you suggest a word that encapsulates what it is that fsmount() returns?
It's almost, but not quite identical with what open(O_PATH) returns, since it
has to be torn down if not actually mounted somewhere when the fd is closed.
> and maybe rename the syscall.
>
> From a very brief reading, I think you are giving it the meaning that would
> be implied by fsstart(2).
Do you have a reference for the manpage for that? Google doesn't seem to find
it.
David
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Pavel Machek @ 2018-07-27 22:01 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E . McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <20180602124408.8430-1-mathieu.desnoyers@efficios.com>
[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]
Hi!
> So for instance, this turns:
>
> int cpu = rseq_per_cpu_lock(lock, target_cpu);
> [...]
> rseq_per_cpu_unlock(lock, cpu);
>
> into
>
> int cpu = rseq_this_cpu_lock(lock);
> [...]
> rseq_per_cpu_unlock(lock, cpu);
>
> and:
>
> per_cpu_list_push(list, node, target_cpu);
> [...]
> per_cpu_list_pop(list, node, target_cpu);
>
> into
>
> this_cpu_list_push(list, node, &cpu); /* cpu is an output parameter. */
> [...]
> node = this_cpu_list_pop(list, &cpu); /* cpu is an output parameter. */
>
> Eventually integrating cpu_opv or some alternative will allow passing
> the cpu number as parameter rather than requiring the algorithm to work
> on the current CPU.
>
> The second effect of not having the cpu_opv fallback is that
> line and instruction single-stepping with a debugger transforms rseq
> critical sections based on retry loops into never-ending loops.
> Debuggers need to use the __rseq_table section to skip those critical
> sections in order to correctly behave when single-stepping a thread
> which uses rseq in a retry loop. However, applications which use an
> alternative fallback method rather than retrying on rseq fast-path abort
> won't be affected by this kind of single-stepping issue.
>
> Thanks for your feedback!
Would it make sense to include Documentation/ patch? I guess at least
manpage describing the syscall will be needed....
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: Andy Lutomirski @ 2018-07-27 21:57 UTC (permalink / raw)
To: David Howells; +Cc: Al Viro, Linux API, Linus Torvalds, Linux FS Devel, LKML
In-Reply-To: <7034.1532728300@warthog.procyon.org.uk>
On Fri, Jul 27, 2018 at 2:51 PM, David Howells <dhowells@redhat.com> wrote:
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> Unless I'm rather confused, you have two or possibly three ways to
>> pass in an open fd. Can you clarify what the difference is and/or
>> remove all but one of them?
>
> No, they're not equivalent.
>
>> > (*) fsconfig_set_path: A non-empty path is specified. The parameter must
>> > be expecting a path object. value points to a NUL-terminated string
>> > that is the path and aux is a file descriptor at which to start a
>> > relative lookup or AT_FDCWD.
>
> So, an example:
>
> fsconfig(fd, fsconfig_set_path, "source", "/dev/sda1", AT_FDCWD);
>
> I don't want to require that the caller open /dev/sda1 and pass in an fd as
> that might prevent the filesystem from "holding" it exclusively.
>
>> > (*) fsconfig_set_path_empty: As fsconfig_set_path, but with AT_EMPTY_PATH
>> > implied.
>
> You can't do:
>
> fsconfig(fd, fsconfig_set_path, "source", "", dir_fd);
>
> because AT_EMPTY_PATH cannot be specified directly[*]. What you do instead is:
>
> fsconfig(fd, fsconfig_set_path_empty, "source", "", dir_fd);
>
> [*] Not without a 6-arg syscall or some other way of passing it.
Are there still architectures that have problems with 6-arg syscalls?
>
> I *could* require that the caller must call open(O_PATH) or openat(O_PATH)
> before calling fsconfig() - so you don't pass a string, but only a path-fd.
>
>> > (*) fsconfig_set_fd: An open file descriptor is specified. value must
>> > be NULL and aux indicates the file descriptor.
>
> See fd=%u on fuse. I think it's cleaner to do:
>
> fsconfig(fd, fsconfig_set_fd, "source", NULL, control_fd);
>
> saying explicitly that there's an open file to be passed rather than:
>
> fsconfig(fd, fsconfig_set_path, "source", NULL, control_fd);
Hmm. That should probably be clearly documented. I suppose that, as
long as there is never a case where fsconfig_set_path and
fsconfig_set_fd both succeed, then it's not a big deal.
^ permalink raw reply
* Re: [PATCH 29/38] vfs: syscall: Add fsconfig() for configuring and managing a context [ver #10]
From: David Howells @ 2018-07-27 21:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Al Viro, Linux API, Linus Torvalds, Linux FS Devel,
LKML
In-Reply-To: <CALCETrUXsOexKcj+K_v_bsW6DkGFgyNOaEs_qVOAcdNGBbEb2w@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> wrote:
> Unless I'm rather confused, you have two or possibly three ways to
> pass in an open fd. Can you clarify what the difference is and/or
> remove all but one of them?
No, they're not equivalent.
> > (*) fsconfig_set_path: A non-empty path is specified. The parameter must
> > be expecting a path object. value points to a NUL-terminated string
> > that is the path and aux is a file descriptor at which to start a
> > relative lookup or AT_FDCWD.
So, an example:
fsconfig(fd, fsconfig_set_path, "source", "/dev/sda1", AT_FDCWD);
I don't want to require that the caller open /dev/sda1 and pass in an fd as
that might prevent the filesystem from "holding" it exclusively.
> > (*) fsconfig_set_path_empty: As fsconfig_set_path, but with AT_EMPTY_PATH
> > implied.
You can't do:
fsconfig(fd, fsconfig_set_path, "source", "", dir_fd);
because AT_EMPTY_PATH cannot be specified directly[*]. What you do instead is:
fsconfig(fd, fsconfig_set_path_empty, "source", "", dir_fd);
[*] Not without a 6-arg syscall or some other way of passing it.
I *could* require that the caller must call open(O_PATH) or openat(O_PATH)
before calling fsconfig() - so you don't pass a string, but only a path-fd.
> > (*) fsconfig_set_fd: An open file descriptor is specified. value must
> > be NULL and aux indicates the file descriptor.
See fd=%u on fuse. I think it's cleaner to do:
fsconfig(fd, fsconfig_set_fd, "source", NULL, control_fd);
saying explicitly that there's an open file to be passed rather than:
fsconfig(fd, fsconfig_set_path, "source", NULL, control_fd);
which indicates that you are actually providing a path.
David
^ 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