* [PATCH v4 3/3] fs: Add aio iopriority support for block_dev
From: adam.manzanares @ 2018-05-17 20:38 UTC (permalink / raw)
To: viro, linux-fsdevel, axboe, bcrl
Cc: mingo, peterz, pombredanne, gregkh, bigeasy, rgoldwyn,
linux-block, linux-kernel, linux-aio, linux-api, Adam Manzanares
In-Reply-To: <20180517203803.2664-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
This is the per-I/O equivalent of the ioprio_set system call.
When IOCB_FLAG_IOPRIO is set on the iocb aio_flags field, then we set the
newly added kiocb ki_ioprio field to the value in the iocb aio_reqprio field.
When a bio is created for an aio request by the block dev we set the priority
value of the bio to the user supplied value.
This patch depends on block: add ioprio_check_cap function
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
fs/aio.c | 16 ++++++++++++++++
fs/block_dev.c | 2 ++
include/linux/fs.h | 2 ++
include/uapi/linux/aio_abi.h | 1 +
4 files changed, 21 insertions(+)
diff --git a/fs/aio.c b/fs/aio.c
index f3eae5d5771b..ff3107aa82d5 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1451,6 +1451,22 @@ static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
if (iocb->aio_flags & IOCB_FLAG_RESFD)
req->ki_flags |= IOCB_EVENTFD;
req->ki_hint = file_write_hint(req->ki_filp);
+ if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
+ /*
+ * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
+ * aio_reqprio is interpreted as an I/O scheduling
+ * class and priority.
+ */
+ ret = ioprio_check_cap(iocb->aio_reqprio);
+ if (ret) {
+ pr_debug("aio ioprio check cap error\n");
+ return -EINVAL;
+ }
+
+ req->ki_ioprio = iocb->aio_reqprio;
+ req->ki_flags |= IOCB_IOPRIO;
+ }
+
ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
if (unlikely(ret))
fput(req->ki_filp);
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..970bef79caa6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -355,6 +355,8 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
bio->bi_write_hint = iocb->ki_hint;
bio->bi_private = dio;
bio->bi_end_io = blkdev_bio_end_io;
+ if (iocb->ki_flags & IOCB_IOPRIO)
+ bio->bi_ioprio = iocb->ki_ioprio;
ret = bio_iov_iter_get_pages(bio, iter);
if (unlikely(ret)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7a90ce387e00..3415e83f6350 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -294,6 +294,7 @@ enum rw_hint {
#define IOCB_SYNC (1 << 5)
#define IOCB_WRITE (1 << 6)
#define IOCB_NOWAIT (1 << 7)
+#define IOCB_IOPRIO (1 << 8)
struct kiocb {
struct file *ki_filp;
@@ -302,6 +303,7 @@ struct kiocb {
void *private;
int ki_flags;
u16 ki_hint;
+ u16 ki_ioprio; /* See linux/ioprio.h */
} __randomize_layout;
static inline bool is_sync_kiocb(struct kiocb *kiocb)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 2c0a3415beee..d4e768d55d14 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -55,6 +55,7 @@ enum {
* is valid.
*/
#define IOCB_FLAG_RESFD (1 << 0)
+#define IOCB_FLAG_IOPRIO (1 << 1)
/* read() from /dev/aio returns these structures. */
struct io_event {
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH v4 2/3] fs: Convert kiocb rw_hint from enum to u16
From: adam.manzanares @ 2018-05-17 20:38 UTC (permalink / raw)
To: viro, linux-fsdevel, axboe, bcrl
Cc: mingo, peterz, pombredanne, gregkh, bigeasy, rgoldwyn,
linux-block, linux-kernel, linux-aio, linux-api, Adam Manzanares
In-Reply-To: <20180517203803.2664-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
In order to avoid kiocb bloat for per command iopriority support, rw_hint
is converted from enum to a u16. Added a guard around ki_hint assigment.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
include/linux/fs.h | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..7a90ce387e00 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -284,6 +284,8 @@ enum rw_hint {
WRITE_LIFE_EXTREME = RWH_WRITE_LIFE_EXTREME,
};
+#define MAX_KI_HINT ((1 << 16) - 1) /* ki_hint type is u16 */
+
#define IOCB_EVENTFD (1 << 0)
#define IOCB_APPEND (1 << 1)
#define IOCB_DIRECT (1 << 2)
@@ -299,7 +301,7 @@ struct kiocb {
void (*ki_complete)(struct kiocb *iocb, long ret, long ret2);
void *private;
int ki_flags;
- enum rw_hint ki_hint;
+ u16 ki_hint;
} __randomize_layout;
static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1927,12 +1929,21 @@ static inline enum rw_hint file_write_hint(struct file *file)
static inline int iocb_flags(struct file *file);
+/* ki_hint changed from enum to u16, make sure rw_hint fits into u16 */
+static inline u16 ki_hint_valid(enum rw_hint hint)
+{
+ if (hint > MAX_KI_HINT)
+ return 0;
+
+ return hint;
+}
+
static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
{
*kiocb = (struct kiocb) {
.ki_filp = filp,
.ki_flags = iocb_flags(filp),
- .ki_hint = file_write_hint(filp),
+ .ki_hint = ki_hint_valid(file_write_hint(filp)),
};
}
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH v4 1/3] block: add ioprio_check_cap function
From: adam.manzanares @ 2018-05-17 20:38 UTC (permalink / raw)
To: viro, linux-fsdevel, axboe, bcrl
Cc: mingo, peterz, pombredanne, gregkh, bigeasy, rgoldwyn,
linux-block, linux-kernel, linux-aio, linux-api, Adam Manzanares
In-Reply-To: <20180517203803.2664-1-adam.manzanares@wdc.com>
From: Adam Manzanares <adam.manzanares@wdc.com>
Aio per command iopriority support introduces a second interface between
userland and the kernel capable of passing iopriority. The aio interface also
needs the ability to verify that the submitting context has sufficient
priviledges to submit IOPRIO_RT commands. This patch creates the
ioprio_check_cap function to be used by the ioprio_set system call and also by
the aio interface.
Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
block/ioprio.c | 22 ++++++++++++++++------
include/linux/ioprio.h | 2 ++
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index 6f5d0b6625e3..f9821080c92c 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -61,15 +61,10 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
}
EXPORT_SYMBOL_GPL(set_task_ioprio);
-SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+int ioprio_check_cap(int ioprio)
{
int class = IOPRIO_PRIO_CLASS(ioprio);
int data = IOPRIO_PRIO_DATA(ioprio);
- struct task_struct *p, *g;
- struct user_struct *user;
- struct pid *pgrp;
- kuid_t uid;
- int ret;
switch (class) {
case IOPRIO_CLASS_RT:
@@ -92,6 +87,21 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
return -EINVAL;
}
+ return 0;
+}
+
+SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+{
+ struct task_struct *p, *g;
+ struct user_struct *user;
+ struct pid *pgrp;
+ kuid_t uid;
+ int ret;
+
+ ret = ioprio_check_cap(ioprio);
+ if (ret)
+ return ret;
+
ret = -ESRCH;
rcu_read_lock();
switch (which) {
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 627efac73e6d..4a28cec49ec3 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -77,4 +77,6 @@ extern int ioprio_best(unsigned short aprio, unsigned short bprio);
extern int set_task_ioprio(struct task_struct *task, int ioprio);
+extern int ioprio_check_cap(int ioprio);
+
#endif
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH v4 0/3] AIO add per-command iopriority
From: adam.manzanares @ 2018-05-17 20:38 UTC (permalink / raw)
To: viro, linux-fsdevel, axboe, bcrl
Cc: mingo, peterz, pombredanne, gregkh, bigeasy, rgoldwyn,
linux-block, linux-kernel, linux-aio, linux-api, Adam Manzanares
From: Adam Manzanares <adam.manzanares@wdc.com>
This is the per-I/O equivalent of the ioprio_set system call.
See the following link for performance implications on a SATA HDD:
https://lkml.org/lkml/2016/12/6/495
First patch factors ioprio_check_cap function out of ioprio_set system call to
also be used by the aio ioprio interface.
Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.
Third patch passes ioprio hint from aio iocb to kiocb and enables block_dev
usage of the per I/O ioprio feature.
v2: merge patches
use IOCB_FLAG_IOPRIO
validate intended use with IOCB_IOPRIO
add linux-api and linux-block to cc
v3: add ioprio_check_cap function
convert kiocb ki_hint to u16
use ioprio_check_cap when adding ioprio to kiocb in aio.c
v4: handle IOCB_IOPRIO in aio_prep_rw
note patch 3 depends on patch 1 in commit msg
Adam Manzanares (3):
block: add ioprio_check_cap function
fs: Convert kiocb rw_hint from enum to u16
fs: Add aio iopriority support for block_dev
block/ioprio.c | 22 ++++++++++++++++------
fs/aio.c | 16 ++++++++++++++++
fs/block_dev.c | 2 ++
include/linux/fs.h | 17 +++++++++++++++--
include/linux/ioprio.h | 2 ++
include/uapi/linux/aio_abi.h | 1 +
6 files changed, 52 insertions(+), 8 deletions(-)
--
2.15.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH] proc: Don't allow empty /proc/PID/cmdline for user tasks
From: Linus Torvalds @ 2018-05-17 18:23 UTC (permalink / raw)
To: Tejun Heo
Cc: Linux Kernel Mailing List, Linux API, Andrew Morton, kernel-team,
Lennart Poettering
In-Reply-To: <20180517012149.GA1718769@devbig577.frc2.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
On Wed, May 16, 2018 at 6:21 PM Tejun Heo <tj@kernel.org> wrote:
> This patch fixes the issue by making proc_pid_cmdline_read() never
> return empty string for user tasks.
Ugh.
That function really is too damn ugly, and this just makes it worse.
Can we please split things up a bit before uglifying the code further?
IOW, the first step should be something like the attached patch, which
splits up all the tsk/mm error cases.
Then your patch could just be a trivial
if (!*pos && !ret)
ret = get_comm_cmdline(tsk, buf, count, pos);
in that new (and much simpler) proc_pid_cmdline_read() just before the
put_task_struct.
Hmm?
NOTE! This patch is *entirely* untested, but it builds and the conversion
was pretty much entirely mechanical.
And yes, the "get_mm_cmdline()" function is still too damn ugly, and should
still be cleaned up more, but it's at least _slightly_ simpler than it used
to be, and the new logic wouldn't go into that horrible thing.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2382 bytes --]
fs/proc/base.c | 64 +++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1a76d751cf3c..c4d963a12162 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -205,11 +205,9 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
return result;
}
-static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
- size_t _count, loff_t *pos)
+static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
+ size_t _count, loff_t *pos)
{
- struct task_struct *tsk;
- struct mm_struct *mm;
char *page;
unsigned long count = _count;
unsigned long arg_start, arg_end, env_start, env_end;
@@ -218,26 +216,13 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
char c;
ssize_t rv;
- BUG_ON(*pos < 0);
-
- tsk = get_proc_task(file_inode(file));
- if (!tsk)
- return -ESRCH;
- mm = get_task_mm(tsk);
- put_task_struct(tsk);
- if (!mm)
- return 0;
/* Check if process spawned far enough to have cmdline. */
- if (!mm->env_end) {
- rv = 0;
- goto out_mmput;
- }
+ if (!mm->env_end)
+ return 0;
page = (char *)__get_free_page(GFP_KERNEL);
- if (!page) {
- rv = -ENOMEM;
- goto out_mmput;
- }
+ if (!page)
+ return -ENOMEM;
down_read(&mm->mmap_sem);
arg_start = mm->arg_start;
@@ -365,13 +350,42 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
out_free_page:
free_page((unsigned long)page);
-out_mmput:
- mmput(mm);
- if (rv > 0)
- *pos += rv;
return rv;
}
+static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,
+ size_t count, loff_t *pos)
+{
+ struct mm_struct *mm;
+ ssize_t ret;
+
+ mm = get_task_mm(tsk);
+ if (!mm)
+ return 0;
+
+ ret = get_mm_cmdline(mm, buf, count, pos);
+ mmput(mm);
+ return ret;
+}
+
+static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
+ size_t count, loff_t *pos)
+{
+ struct task_struct *tsk;
+ ssize_t ret;
+
+ BUG_ON(*pos < 0);
+
+ tsk = get_proc_task(file_inode(file));
+ if (!tsk)
+ return -ESRCH;
+ ret = get_task_cmdline(tsk, buf, count, pos);
+ put_task_struct(tsk);
+ if (ret > 0)
+ *pos += ret;
+ return ret;
+}
+
static const struct file_operations proc_pid_cmdline_ops = {
.read = proc_pid_cmdline_read,
.llseek = generic_file_llseek,
^ permalink raw reply related
* Re: [RFC PATCH] UAPI: Document auxvec AT_* namespace policy and note reservations
From: Yann Droneaud @ 2018-05-17 16:00 UTC (permalink / raw)
To: Dave Martin, linux-kernel
Cc: x86, linux-arch, linux-api, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Russell King, Catalin Marinas, Will Deacon,
Tony Luck, Fenghua Yu, Michal Simek, Ralf Baechle, James Hogan,
Greentime Hu, Vincent Chen, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Palmer Dabbelt, Albert Ou,
Martin
In-Reply-To: <1526480447-18185-1-git-send-email-Dave.Martin@arm.com>
Hi,
Le mercredi 16 mai 2018 à 15:20 +0100, Dave Martin a écrit :
> There are constraints on defining AT_* auxvec tags that are not
> obvious to the casual maintainer of either the global
> <uapi/linux/auxvec.h> or the arch-specific headers. This is likely
> to lead to mistakes. (I certainly fell foul of it...)
>
> For the benefit of future maintainers, this patch collects the
> relevant information in one place, documenting how the namespace
> needs to be managed, and noting all the values currently in use.
>
> Maintaining a global list may result in some merge conflicts, but
> AT_* values are not added frequently. I'm open to suggestions on
> the best approach.
>
> I also assume that values 38 and 39 may have been used for
> historical purposes, such as an architecture that is no longer
> supported. If they have definitely never been used for anything,
> they could be removed from the "reserved" list.
>
Some of those AT_* values are described in getauxval(3) man-page:
http://man7.org/linux/man-pages/man3/getauxval.3.html
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man3/g
etauxval.3?id=4eae8eb731386d81797d5c30365426722410874e
And glibc provides <elf.h> with definitions for almost all AT_*,
regardless of the current target architecture:
https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/elf.h;h=954f3266f7
11ab83996670ea504a17dcf668e061;hb=23158b08a0908f381459f273a984c6fd32836
3cb#l1135
Also, despite not being listed as a reserved namespace by POSIX, one
should try to avoid name collision with other AT_ constants,
those used with *at() functions (openat(), etc.):
- AT_EACCESS
- AT_EMPTY_PATH
- AT_FDCWD
- AT_NO_AUTOMOUNT
- AT_REMOVEDIR
- AT_STATX_DONT_SYNC
- AT_STATX_FORCE_SYNC
- AT_STATX_SYNC_AS_STAT
- AT_SYMLINK_FOLLOW
- AT_SYMLINK_NOFOLLOW
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/fcntl.h.html
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.htm
l#tag_15_02_02
https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fcntl.h;h=3d239e8f0
9f7ce0a3106621be327e1ea4cd1a3e7;hb=23158b08a0908f381459f273a984c6fd3283
63cb#l142
And there's also AT_ANYNET and AT_ANYNODE from ddp (aka. AppleTalk)
http://man7.org/linux/man-pages/man7/ddp.7.html
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH 03/14] arm: Add restartable sequences support
From: Mathieu Desnoyers @ 2018-05-17 15:30 UTC (permalink / raw)
To: Will Deacon
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: <20180517133230.GA2688@arm.com>
----- On May 17, 2018, at 9:32 AM, Will Deacon will.deacon@arm.com wrote:
> On Wed, May 16, 2018 at 04:13:13PM -0400, Mathieu Desnoyers wrote:
>> ----- On May 16, 2018, at 12:18 PM, Peter Zijlstra peterz@infradead.org wrote:
>>
>> > On Mon, Apr 30, 2018 at 06:44:22PM -0400, Mathieu Desnoyers wrote:
>> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> >> index a7f8e7f4b88f..4f5c386631d4 100644
>> >> --- a/arch/arm/Kconfig
>> >> +++ b/arch/arm/Kconfig
>> >> @@ -91,6 +91,7 @@ config ARM
>> >> select HAVE_PERF_USER_STACK_DUMP
>> >> select HAVE_RCU_TABLE_FREE if (SMP && ARM_LPAE)
>> >> select HAVE_REGS_AND_STACK_ACCESS_API
>> >> + select HAVE_RSEQ
>> >> select HAVE_SYSCALL_TRACEPOINTS
>> >> select HAVE_UID16
>> >> select HAVE_VIRT_CPU_ACCOUNTING_GEN
>> >> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
>> >> index bd8810d4acb3..5879ab3f53c1 100644
>> >> --- a/arch/arm/kernel/signal.c
>> >> +++ b/arch/arm/kernel/signal.c
>> >> @@ -541,6 +541,12 @@ static void handle_signal(struct ksignal *ksig, struct
>> >> pt_regs *regs)
>> >> int ret;
>> >>
>> >> /*
>> >> + * Increment event counter and perform fixup for the pre-signal
>> >> + * frame.
>> >> + */
>> >> + rseq_signal_deliver(regs);
>> >> +
>> >> + /*
>> >> * Set up the stack frame
>> >> */
>> >> if (ksig->ka.sa.sa_flags & SA_SIGINFO)
>> >> @@ -660,6 +666,7 @@ do_work_pending(struct pt_regs *regs, unsigned int
>> >> thread_flags, int syscall)
>> >> } else {
>> >> clear_thread_flag(TIF_NOTIFY_RESUME);
>> >> tracehook_notify_resume(regs);
>> >> + rseq_handle_notify_resume(regs);
>> >> }
>> >> }
>> >> local_irq_disable();
>> >
>> > I think you forgot to hook up rseq_syscall() checking.
>>
>> Considering that rseq_syscall is implemented as follows:
>>
>> +void rseq_syscall(struct pt_regs *regs)
>> +{
>> + unsigned long ip = instruction_pointer(regs);
>> + struct task_struct *t = current;
>> + struct rseq_cs rseq_cs;
>> +
>> + if (!t->rseq)
>> + return;
>> + if (!access_ok(VERIFY_READ, t->rseq, sizeof(*t->rseq)) ||
>> + rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
>> + force_sig(SIGSEGV, t);
>> +}
>>
>> and that x86 calls it from syscall_return_slowpath() (which AFAIU is
>> now used in the fast-path since KPTI), I wonder where we should call
>> this on ARM ? I was under the impression that ARM return to userspace
>> fast-path was not calling C code unless work flags were set, but I might
>> be wrong.
>>
>> Thoughts ?
>
> Since this only matters for CONFIG_DEBUG_RSEQ, can we just force the
> slowpath for rseq tasks when that option is set?
Or as proposed by Boqun, we can simply call rseq_syscall in a CONFIG_DEBUG_RSEQ
ifdef. Given that this is a debug option, is it worth it to add the current->rseq
test for NULL in assembly before the call, or do we want to favor simplicity ?
Thanks,
Mathieu
>
> Will
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 07/14] powerpc: Add support for restartable sequences
From: Mathieu Desnoyers @ 2018-05-17 15:28 UTC (permalink / raw)
To: Boqun Feng, Will Deacon
Cc: Peter Zijlstra, Paul E. McKenney, 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, Michael
In-Reply-To: <20180517011949.GA1121@tardis>
----- On May 16, 2018, at 9:19 PM, Boqun Feng boqun.feng@gmail.com wrote:
> On Wed, May 16, 2018 at 04:13:16PM -0400, Mathieu Desnoyers wrote:
>> ----- On May 16, 2018, at 12:18 PM, Peter Zijlstra peterz@infradead.org wrote:
>>
>> > On Mon, Apr 30, 2018 at 06:44:26PM -0400, Mathieu Desnoyers wrote:
>> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> >> index c32a181a7cbb..ed21a777e8c6 100644
>> >> --- a/arch/powerpc/Kconfig
>> >> +++ b/arch/powerpc/Kconfig
>> >> @@ -223,6 +223,7 @@ config PPC
>> >> select HAVE_SYSCALL_TRACEPOINTS
>> >> select HAVE_VIRT_CPU_ACCOUNTING
>> >> select HAVE_IRQ_TIME_ACCOUNTING
>> >> + select HAVE_RSEQ
>> >> select IRQ_DOMAIN
>> >> select IRQ_FORCED_THREADING
>> >> select MODULES_USE_ELF_RELA
>> >> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
>> >> index 61db86ecd318..d3bb3aaaf5ac 100644
>> >> --- a/arch/powerpc/kernel/signal.c
>> >> +++ b/arch/powerpc/kernel/signal.c
>> >> @@ -133,6 +133,8 @@ static void do_signal(struct task_struct *tsk)
>> >> /* Re-enable the breakpoints for the signal stack */
>> >> thread_change_pc(tsk, tsk->thread.regs);
>> >>
>> >> + rseq_signal_deliver(tsk->thread.regs);
>> >> +
>> >> if (is32) {
>> >> if (ksig.ka.sa.sa_flags & SA_SIGINFO)
>> >> ret = handle_rt_signal32(&ksig, oldset, tsk);
>> >> @@ -164,6 +166,7 @@ void do_notify_resume(struct pt_regs *regs, unsigned long
>> >> thread_info_flags)
>> >> if (thread_info_flags & _TIF_NOTIFY_RESUME) {
>> >> clear_thread_flag(TIF_NOTIFY_RESUME);
>> >> tracehook_notify_resume(regs);
>> >> + rseq_handle_notify_resume(regs);
>> >> }
>> >>
>> >> user_enter();
>> >
>> > Again no rseq_syscall().
>>
>> Same question for PowerPC as for ARM:
>>
>> Considering that rseq_syscall is implemented as follows:
>>
>> +void rseq_syscall(struct pt_regs *regs)
>> +{
>> + unsigned long ip = instruction_pointer(regs);
>> + struct task_struct *t = current;
>> + struct rseq_cs rseq_cs;
>> +
>> + if (!t->rseq)
>> + return;
>> + if (!access_ok(VERIFY_READ, t->rseq, sizeof(*t->rseq)) ||
>> + rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
>> + force_sig(SIGSEGV, t);
>> +}
>>
>> and that x86 calls it from syscall_return_slowpath() (which AFAIU is
>> now used in the fast-path since KPTI), I wonder where we should call
>
> So we actually detect this after the syscall takes effect, right? I
> wonder whether this could be problematic, because "disallowing syscall"
> in rseq areas may means the syscall won't take effect to some people, I
> guess?
>
>> this on PowerPC ? I was under the impression that PowerPC return to
>> userspace fast-path was not calling C code unless work flags were set,
>> but I might be wrong.
>>
>
> I think you're right. So we have to introduce callsite to rseq_syscall()
> in syscall path, something like:
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 51695608c68b..a25734a96640 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -222,6 +222,9 @@ system_call_exit:
> mtmsrd r11,1
> #endif /* CONFIG_PPC_BOOK3E */
>
> + addi r3,r1,STACK_FRAME_OVERHEAD
> + bl rseq_syscall
> +
> ld r9,TI_FLAGS(r12)
> li r11,-MAX_ERRNO
> andi.
> r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
>
> But I think it's important for us to first decide where (before or after
> the syscall) we do the detection.
As Peter said, we don't really care whether it's on syscall entry or exit, as
long as the process gets killed when the erroneous use is detected. I think doing
it on syscall exit is a bit easier because we can clearly access the userspace
TLS, which AFAIU may be less straightforward on syscall entry.
We may want to add #ifdef CONFIG_DEBUG_RSEQ / #endif around the code you
proposed above, so it's only compiled in if CONFIG_DEBUG_RSEQ=y.
On the ARM leg of the email thread, Will Deacon suggests to test whether current->rseq
is non-NULL before calling rseq_syscall(). I wonder if this added check is justified
as the assembly level, considering that this is just a debugging option. We already do
that check at the very beginning of rseq_syscall().
Thoughts ?
Thanks,
Mathieu
>
> Regards,
> Boqun
>
>> Thoughts ?
>>
>> Thanks!
>>
>> Mathieu
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 03/14] arm: Add restartable sequences support
From: Will Deacon @ 2018-05-17 13:32 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, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <670368504.1912.1526501593893.JavaMail.zimbra@efficios.com>
On Wed, May 16, 2018 at 04:13:13PM -0400, Mathieu Desnoyers wrote:
> ----- On May 16, 2018, at 12:18 PM, Peter Zijlstra peterz@infradead.org wrote:
>
> > On Mon, Apr 30, 2018 at 06:44:22PM -0400, Mathieu Desnoyers wrote:
> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> index a7f8e7f4b88f..4f5c386631d4 100644
> >> --- a/arch/arm/Kconfig
> >> +++ b/arch/arm/Kconfig
> >> @@ -91,6 +91,7 @@ config ARM
> >> select HAVE_PERF_USER_STACK_DUMP
> >> select HAVE_RCU_TABLE_FREE if (SMP && ARM_LPAE)
> >> select HAVE_REGS_AND_STACK_ACCESS_API
> >> + select HAVE_RSEQ
> >> select HAVE_SYSCALL_TRACEPOINTS
> >> select HAVE_UID16
> >> select HAVE_VIRT_CPU_ACCOUNTING_GEN
> >> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> >> index bd8810d4acb3..5879ab3f53c1 100644
> >> --- a/arch/arm/kernel/signal.c
> >> +++ b/arch/arm/kernel/signal.c
> >> @@ -541,6 +541,12 @@ static void handle_signal(struct ksignal *ksig, struct
> >> pt_regs *regs)
> >> int ret;
> >>
> >> /*
> >> + * Increment event counter and perform fixup for the pre-signal
> >> + * frame.
> >> + */
> >> + rseq_signal_deliver(regs);
> >> +
> >> + /*
> >> * Set up the stack frame
> >> */
> >> if (ksig->ka.sa.sa_flags & SA_SIGINFO)
> >> @@ -660,6 +666,7 @@ do_work_pending(struct pt_regs *regs, unsigned int
> >> thread_flags, int syscall)
> >> } else {
> >> clear_thread_flag(TIF_NOTIFY_RESUME);
> >> tracehook_notify_resume(regs);
> >> + rseq_handle_notify_resume(regs);
> >> }
> >> }
> >> local_irq_disable();
> >
> > I think you forgot to hook up rseq_syscall() checking.
>
> Considering that rseq_syscall is implemented as follows:
>
> +void rseq_syscall(struct pt_regs *regs)
> +{
> + unsigned long ip = instruction_pointer(regs);
> + struct task_struct *t = current;
> + struct rseq_cs rseq_cs;
> +
> + if (!t->rseq)
> + return;
> + if (!access_ok(VERIFY_READ, t->rseq, sizeof(*t->rseq)) ||
> + rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
> + force_sig(SIGSEGV, t);
> +}
>
> and that x86 calls it from syscall_return_slowpath() (which AFAIU is
> now used in the fast-path since KPTI), I wonder where we should call
> this on ARM ? I was under the impression that ARM return to userspace
> fast-path was not calling C code unless work flags were set, but I might
> be wrong.
>
> Thoughts ?
Since this only matters for CONFIG_DEBUG_RSEQ, can we just force the
slowpath for rseq tasks when that option is set?
Will
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Florian Weimer @ 2018-05-17 10:11 UTC (permalink / raw)
To: Ram Pai
Cc: linux-arch, Linux-MM, Linux API, X86 ML, Dave Hansen,
linux-x86_64, Andy Lutomirski, linuxppc-dev
In-Reply-To: <20180516203534.GA5479@ram.oc3035372033.ibm.com>
On 05/16/2018 10:35 PM, Ram Pai wrote:
> So let me see if I understand the overall idea.
>
> Application can allocate new keys through a new syscall
> sys_pkey_alloc_1(flags, init_val, sig_init_val)
>
> 'sig_init_val' is the permission-state of the key in signal context.
I would keep the existing system call and just add a flag, say
PKEY_ALLOC_SETSIGNAL. If the current thread needs different access
rights, it can set those rights just after pkey_alloc returns. There is
no race that matters here, I think.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Florian Weimer @ 2018-05-17 10:09 UTC (permalink / raw)
To: Ram Pai, Andy Lutomirski
Cc: linux-arch, Linux-MM, Linux API, X86 ML, Dave Hansen,
linux-x86_64, linuxppc-dev
In-Reply-To: <20180516210745.GC5479@ram.oc3035372033.ibm.com>
On 05/16/2018 11:07 PM, Ram Pai wrote:
> what would change the key-permission-values enforced in signal-handler
> context? Or can it never be changed, ones set through sys_pkey_alloc()?
The access rights can only be set by pkey_alloc and are unchanged after
that (so we do not have to discuss whether the signal handler access
rights are per-thread or not).
> I suppose key-permission-values change done in non-signal-handler context,
> will not apply to those in signal-handler context.
Correct, that is the plan.
> Can the signal handler change the key-permission-values from the
> signal-handler context?
Yes, changes are possible. The access rights given to pkey_alloc only
specify the initial access rights when the signal handler is entered.
We need to decide if we should restore it on exit from the signal
handler. There is also the matter of siglongjmp, which currently does
not restore the current thread's access rights. In general, this might
be difficult to implement because of the limited space in jmp_buf.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH 07/14] powerpc: Add support for restartable sequences
From: Peter Zijlstra @ 2018-05-17 7:43 UTC (permalink / raw)
To: Boqun Feng
Cc: Mathieu Desnoyers, Paul E. McKenney, 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
In-Reply-To: <20180517011949.GA1121@tardis>
On Thu, May 17, 2018 at 09:19:49AM +0800, Boqun Feng wrote:
> On Wed, May 16, 2018 at 04:13:16PM -0400, Mathieu Desnoyers wrote:
> > and that x86 calls it from syscall_return_slowpath() (which AFAIU is
> > now used in the fast-path since KPTI), I wonder where we should call
>
> So we actually detect this after the syscall takes effect, right? I
> wonder whether this could be problematic, because "disallowing syscall"
> in rseq areas may means the syscall won't take effect to some people, I
> guess?
It doesn't really matter I suspect, the important part is the program
getting killed.
I agree that doing it on sysenter is slightly nicer, but I'll take
sysexit if that's what it takes.
> > this on PowerPC ? I was under the impression that PowerPC return to
> > userspace fast-path was not calling C code unless work flags were set,
> > but I might be wrong.
> >
>
> I think you're right. So we have to introduce callsite to rseq_syscall()
> in syscall path, something like:
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 51695608c68b..a25734a96640 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -222,6 +222,9 @@ system_call_exit:
> mtmsrd r11,1
> #endif /* CONFIG_PPC_BOOK3E */
>
> + addi r3,r1,STACK_FRAME_OVERHEAD
> + bl rseq_syscall
> +
> ld r9,TI_FLAGS(r12)
> li r11,-MAX_ERRNO
> andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
>
> But I think it's important for us to first decide where (before or after
> the syscall) we do the detection.
The important thing is the processed getting very dead. Either sysenter
or sysexit gets that done.
^ permalink raw reply
* Re: [RFC PATCH] UAPI: Document auxvec AT_* namespace policy and note reservations
From: Michael Ellerman @ 2018-05-17 6:40 UTC (permalink / raw)
To: Dave Martin, linux-kernel
Cc: x86, linux-arch, linux-api, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Russell King, Catalin Marinas, Will Deacon,
Tony Luck, Fenghua Yu, Michal Simek, Ralf Baechle, James Hogan,
Greentime Hu, Vincent Chen, Benjamin Herrenschmidt,
Paul Mackerras, Palmer Dabbelt, Albert Ou, Martin Schwidefsky,
Hei
In-Reply-To: <1526480447-18185-1-git-send-email-Dave.Martin@arm.com>
Dave Martin <Dave.Martin@arm.com> writes:
> There are constraints on defining AT_* auxvec tags that are not
> obvious to the casual maintainer of either the global
> <uapi/linux/auxvec.h> or the arch-specific headers. This is likely
> to lead to mistakes. (I certainly fell foul of it...)
Thanks for cleaning this up.
It looks like us (powerpc) / me is the main offender here.
My excuse is it was glibc folk who asked us to add all those new AT_
entries in the first place. </buckpassing>
> For the benefit of future maintainers, this patch collects the
> relevant information in one place, documenting how the namespace
> needs to be managed, and noting all the values currently in use.
>
> Maintaining a global list may result in some merge conflicts, but
> AT_* values are not added frequently. I'm open to suggestions on
> the best approach.
Yeah I agree with Rich that having a global list would be best. That is
the most reliable to make people think twice about adding new entries.
> I also assume that values 38 and 39 may have been used for
> historical purposes, such as an architecture that is no longer
> supported. If they have definitely never been used for anything,
> they could be removed from the "reserved" list.
I don't know why we added the new entries starting at 40, maybe Ben
remembers. Quite likely it was just an accident.
I don't see any sign of 38 or 39 in glibc history.
cheers
^ permalink raw reply
* [PATCH 6/6] workqueue: Show the latest workqueue name in /proc/PID/{comm,stat,status}
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
There can be a lot of workqueue workers and they all show up with the
cryptic kworker/* names making it difficult to understand which is
doing what and how they came to be.
# ps -ef | grep kworker
root 4 2 0 Feb25 ? 00:00:00 [kworker/0:0H]
root 6 2 0 Feb25 ? 00:00:00 [kworker/u112:0]
root 19 2 0 Feb25 ? 00:00:00 [kworker/1:0H]
root 25 2 0 Feb25 ? 00:00:00 [kworker/2:0H]
root 31 2 0 Feb25 ? 00:00:00 [kworker/3:0H]
...
This patch makes workqueue workers report the latest workqueue it was
executing for through /proc/PID/{comm,stat,status}. The extra
information is appended to the kthread name with intervening '+' if
currently executing, otherwise '-'.
# cat /proc/25/comm
kworker/2:0-events_power_efficient
# cat /proc/25/stat
25 (kworker/2:0-events_power_efficient) I 2 0 0 0 -1 69238880 0 0...
# grep Name /proc/25/status
Name: kworker/2:0-events_power_efficient
Unfortunately, ps(1) truncates comm to 15 characters,
# ps 25
PID TTY STAT TIME COMMAND
25 ? I 0:00 [kworker/2:0-eve]
making it a lot less useful; however, this should be an easy fix from
ps(1) side.
Signed-off-by: Tejun Heo <tj@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Craig Small <csmall@enc.com.au>
---
fs/proc/array.c | 7 +++++--
include/linux/workqueue.h | 1 +
kernel/workqueue.c | 39 +++++++++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index f29221e..bb1d361 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -99,10 +99,13 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
{
char *buf;
size_t size;
- char tcomm[sizeof(p->comm)];
+ char tcomm[64];
int ret;
- get_task_comm(tcomm, p);
+ if (p->flags & PF_WQ_WORKER)
+ wq_worker_comm(tcomm, sizeof(tcomm), p);
+ else
+ __get_task_comm(tcomm, sizeof(tcomm), p);
size = seq_get_buf(m, &buf);
if (escape) {
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 39a0e21..60d673e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -494,6 +494,7 @@ extern unsigned int work_busy(struct work_struct *work);
extern __printf(1, 2) void set_worker_desc(const char *fmt, ...);
extern void print_worker_info(const char *log_lvl, struct task_struct *task);
extern void show_workqueue_state(void);
+extern void wq_worker_comm(char *buf, size_t size, struct task_struct *task);
/**
* queue_work - queue work on a workqueue
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 3fbe007..b4a39a1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4577,6 +4577,45 @@ void show_workqueue_state(void)
rcu_read_unlock_sched();
}
+/* used to show worker information through /proc/PID/{comm,stat,status} */
+void wq_worker_comm(char *buf, size_t size, struct task_struct *task)
+{
+ struct worker *worker;
+ struct worker_pool *pool;
+ int off;
+
+ /* always show the actual comm */
+ off = strscpy(buf, task->comm, size);
+ if (off < 0)
+ return;
+
+ /* stabilize worker pool association */
+ mutex_lock(&wq_pool_attach_mutex);
+
+ worker = kthread_data(task);
+ pool = worker->pool;
+
+ if (pool) {
+ spin_lock_irq(&pool->lock);
+ /*
+ * ->desc tracks information (wq name or set_worker_desc())
+ * for the latest execution. If current, prepend '+',
+ * otherwise '-'.
+ */
+ if (worker->desc[0] != '\0') {
+ if (worker->current_work)
+ scnprintf(buf + off, size - off, "+%s",
+ worker->desc);
+ else
+ scnprintf(buf + off, size - off, "-%s",
+ worker->desc);
+ }
+ spin_unlock_irq(&pool->lock);
+ }
+
+ mutex_unlock(&wq_pool_attach_mutex);
+}
+
/*
* CPU hotplug.
*
--
2.9.5
^ permalink raw reply related
* [PATCH 5/6] proc: Consolidate task->comm formatting into proc_task_name()
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
proc shows task->comm in three places - comm, stat, status - and each
is fetching and formatting task->comm slighly differently. This patch
renames task_name() to proc_task_name(), makes it more generic, and
updates all three paths to use it.
This will enable expanding comm reporting for workqueue workers.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
fs/proc/array.c | 26 +++++++++++++++-----------
fs/proc/base.c | 5 ++---
fs/proc/internal.h | 2 ++
3 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index ae2c807..f29221e 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -95,7 +95,7 @@
#include <asm/processor.h>
#include "internal.h"
-static inline void task_name(struct seq_file *m, struct task_struct *p)
+void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
{
char *buf;
size_t size;
@@ -104,13 +104,17 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
get_task_comm(tcomm, p);
- seq_puts(m, "Name:\t");
-
size = seq_get_buf(m, &buf);
- ret = string_escape_str(tcomm, buf, size, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
- seq_commit(m, ret < size ? ret : -1);
+ if (escape) {
+ ret = string_escape_str(tcomm, buf, size,
+ ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
+ if (ret >= size)
+ ret = -1;
+ } else {
+ ret = strscpy(buf, tcomm, size);
+ }
- seq_putc(m, '\n');
+ seq_commit(m, ret);
}
/*
@@ -365,7 +369,10 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
{
struct mm_struct *mm = get_task_mm(task);
- task_name(m, task);
+ seq_puts(m, "Name:\t");
+ proc_task_name(m, task, true);
+ seq_putc(m, '\n');
+
task_state(m, ns, pid, task);
if (mm) {
@@ -400,7 +407,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
u64 cutime, cstime, utime, stime;
u64 cgtime, gtime;
unsigned long rsslim = 0;
- char tcomm[sizeof(task->comm)];
unsigned long flags;
state = *get_task_state(task);
@@ -427,8 +433,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
}
}
- get_task_comm(tcomm, task);
-
sigemptyset(&sigign);
sigemptyset(&sigcatch);
cutime = cstime = utime = stime = 0;
@@ -495,7 +499,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
seq_put_decimal_ull(m, "", pid_nr_ns(pid, ns));
seq_puts(m, " (");
- seq_puts(m, tcomm);
+ proc_task_name(m, task, false);
seq_puts(m, ") ");
seq_putc(m, state);
seq_put_decimal_ll(m, " ", ppid);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2eee4d7..eb17917ca 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1581,9 +1581,8 @@ static int comm_show(struct seq_file *m, void *v)
if (!p)
return -ESRCH;
- task_lock(p);
- seq_printf(m, "%s\n", p->comm);
- task_unlock(p);
+ proc_task_name(m, p, false);
+ seq_putc(m, '\n');
put_task_struct(p);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 0f1692e..b823fac62 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -131,6 +131,8 @@ unsigned name_to_int(const struct qstr *qstr);
*/
extern const struct file_operations proc_tid_children_operations;
+extern void proc_task_name(struct seq_file *m, struct task_struct *p,
+ bool escape);
extern int proc_tid_stat(struct seq_file *, struct pid_namespace *,
struct pid *, struct task_struct *);
extern int proc_tgid_stat(struct seq_file *, struct pid_namespace *,
--
2.9.5
^ permalink raw reply related
* [PATCH 4/6] workqueue: Set worker->desc to workqueue name by default
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
Work functions can use set_worker_desc() to improve the visibility of
what the worker task is doing. Currently, the desc field is unset at
the beginning of each execution and there is a separate field to track
the field is set during the current execution.
Instead of leaving empty till desc is set, worker->desc can be used to
remember the last workqueue the worker worked on by default and users
that use set_worker_desc() can override it to something more
informative as necessary.
This simplifies desc handling and helps tracking the last workqueue
that the worker exected on to improve visibility.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/workqueue.c | 21 ++++++++++-----------
kernel/workqueue_internal.h | 1 -
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2fde50f..3fbe007 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2088,6 +2088,12 @@ __acquires(&pool->lock)
worker->current_pwq = pwq;
work_color = get_work_color(work);
+ /*
+ * Record wq name for cmdline and debug reporting, may get
+ * overridden through set_worker_desc().
+ */
+ strscpy(worker->desc, pwq->wq->name, WORKER_DESC_LEN);
+
list_del_init(&work->entry);
/*
@@ -2183,7 +2189,6 @@ __acquires(&pool->lock)
worker->current_work = NULL;
worker->current_func = NULL;
worker->current_pwq = NULL;
- worker->desc_valid = false;
pwq_dec_nr_in_flight(pwq, work_color);
}
@@ -4346,7 +4351,6 @@ void set_worker_desc(const char *fmt, ...)
va_start(args, fmt);
vsnprintf(worker->desc, sizeof(worker->desc), fmt, args);
va_end(args);
- worker->desc_valid = true;
}
}
@@ -4370,7 +4374,6 @@ void print_worker_info(const char *log_lvl, struct task_struct *task)
char desc[WORKER_DESC_LEN] = { };
struct pool_workqueue *pwq = NULL;
struct workqueue_struct *wq = NULL;
- bool desc_valid = false;
struct worker *worker;
if (!(task->flags & PF_WQ_WORKER))
@@ -4383,22 +4386,18 @@ void print_worker_info(const char *log_lvl, struct task_struct *task)
worker = kthread_probe_data(task);
/*
- * Carefully copy the associated workqueue's workfn and name. Keep
- * the original last '\0' in case the original contains garbage.
+ * Carefully copy the associated workqueue's workfn, name and desc.
+ * Keep the original last '\0' in case the original is garbage.
*/
probe_kernel_read(&fn, &worker->current_func, sizeof(fn));
probe_kernel_read(&pwq, &worker->current_pwq, sizeof(pwq));
probe_kernel_read(&wq, &pwq->wq, sizeof(wq));
probe_kernel_read(name, wq->name, sizeof(name) - 1);
-
- /* copy worker description */
- probe_kernel_read(&desc_valid, &worker->desc_valid, sizeof(desc_valid));
- if (desc_valid)
- probe_kernel_read(desc, worker->desc, sizeof(desc) - 1);
+ probe_kernel_read(desc, worker->desc, sizeof(desc) - 1);
if (fn || name[0] || desc[0]) {
printk("%sWorkqueue: %s %pf", log_lvl, name, fn);
- if (desc[0])
+ if (strcmp(name, desc))
pr_cont(" (%s)", desc);
pr_cont("\n");
}
diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
index 4a182e0..66fbb5a 100644
--- a/kernel/workqueue_internal.h
+++ b/kernel/workqueue_internal.h
@@ -31,7 +31,6 @@ struct worker {
struct work_struct *current_work; /* L: work being processed */
work_func_t current_func; /* L: current_work's fn */
struct pool_workqueue *current_pwq; /* L: current_work's pwq */
- bool desc_valid; /* ->desc is valid */
struct list_head scheduled; /* L: scheduled works */
/* 64 bytes boundary on 64bit, 32 on 32bit */
--
2.9.5
^ permalink raw reply related
* [PATCH 3/6] workqueue: Make worker_attach/detach_pool() update worker->pool
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
For historical reasons, the worker attach/detach functions don't
currently manage worker->pool and the callers are manually and
inconsistently updating it.
This patch moves worker->pool updates into the worker attach/detach
functions. This makes worker->pool consistent and clearly defines how
worker->pool updates are synchronized.
This will help later workqueue visibility improvements by allowing
safe access to workqueue information from worker->task.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/workqueue.c | 16 ++++++++--------
kernel/workqueue_internal.h | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 91fe0a6..2fde50f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1741,6 +1741,7 @@ static void worker_attach_to_pool(struct worker *worker,
worker->flags |= WORKER_UNBOUND;
list_add_tail(&worker->node, &pool->workers);
+ worker->pool = pool;
mutex_unlock(&wq_pool_attach_mutex);
}
@@ -1748,19 +1749,21 @@ static void worker_attach_to_pool(struct worker *worker,
/**
* worker_detach_from_pool() - detach a worker from its pool
* @worker: worker which is attached to its pool
- * @pool: the pool @worker is attached to
*
* Undo the attaching which had been done in worker_attach_to_pool(). The
* caller worker shouldn't access to the pool after detached except it has
* other reference to the pool.
*/
-static void worker_detach_from_pool(struct worker *worker,
- struct worker_pool *pool)
+static void worker_detach_from_pool(struct worker *worker)
{
+ struct worker_pool *pool = worker->pool;
struct completion *detach_completion = NULL;
mutex_lock(&wq_pool_attach_mutex);
+
list_del(&worker->node);
+ worker->pool = NULL;
+
if (list_empty(&pool->workers))
detach_completion = pool->detach_completion;
mutex_unlock(&wq_pool_attach_mutex);
@@ -1799,7 +1802,6 @@ static struct worker *create_worker(struct worker_pool *pool)
if (!worker)
goto fail;
- worker->pool = pool;
worker->id = id;
if (pool->cpu >= 0)
@@ -2236,7 +2238,7 @@ static int worker_thread(void *__worker)
set_task_comm(worker->task, "kworker/dying");
ida_simple_remove(&pool->worker_ida, worker->id);
- worker_detach_from_pool(worker, pool);
+ worker_detach_from_pool(worker);
kfree(worker);
return 0;
}
@@ -2367,7 +2369,6 @@ static int rescuer_thread(void *__rescuer)
worker_attach_to_pool(rescuer, pool);
spin_lock_irq(&pool->lock);
- rescuer->pool = pool;
/*
* Slurp in all works issued via this workqueue and
@@ -2417,10 +2418,9 @@ static int rescuer_thread(void *__rescuer)
if (need_more_worker(pool))
wake_up_worker(pool);
- rescuer->pool = NULL;
spin_unlock_irq(&pool->lock);
- worker_detach_from_pool(rescuer, pool);
+ worker_detach_from_pool(rescuer);
spin_lock_irq(&wq_mayday_lock);
}
diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
index d390d1b..4a182e0 100644
--- a/kernel/workqueue_internal.h
+++ b/kernel/workqueue_internal.h
@@ -37,7 +37,7 @@ struct worker {
/* 64 bytes boundary on 64bit, 32 on 32bit */
struct task_struct *task; /* I: worker task */
- struct worker_pool *pool; /* I: the associated pool */
+ struct worker_pool *pool; /* A: the associated pool */
/* L: for rescuers */
struct list_head node; /* A: anchored at pool->workers */
/* A: runs through worker->node */
--
2.9.5
^ permalink raw reply related
* [PATCH 2/6] workqueue: Replace pool->attach_mutex with global wq_pool_attach_mutex
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
To improve workqueue visibility, we want to be able to access
workqueue information from worker tasks. The per-pool attach mutex
makes that difficult because there's no way of stabilizing task ->
worker pool association without knowing the pool first.
Worker attach/detach is a slow path and there's no need for different
pools to be able to perform them concurrently. This patch replaces
the per-pool attach_mutex with global wq_pool_attach_mutex to prepare
for visibility improvement changes.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/workqueue.c | 41 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ca7959b..91fe0a6 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -66,7 +66,7 @@ enum {
* be executing on any CPU. The pool behaves as an unbound one.
*
* Note that DISASSOCIATED should be flipped only while holding
- * attach_mutex to avoid changing binding state while
+ * wq_pool_attach_mutex to avoid changing binding state while
* worker_attach_to_pool() is in progress.
*/
POOL_MANAGER_ACTIVE = 1 << 0, /* being managed */
@@ -123,7 +123,7 @@ enum {
* cpu or grabbing pool->lock is enough for read access. If
* POOL_DISASSOCIATED is set, it's identical to L.
*
- * A: pool->attach_mutex protected.
+ * A: wq_pool_attach_mutex protected.
*
* PL: wq_pool_mutex protected.
*
@@ -166,7 +166,6 @@ struct worker_pool {
/* L: hash of busy workers */
struct worker *manager; /* L: purely informational */
- struct mutex attach_mutex; /* attach/detach exclusion */
struct list_head workers; /* A: attached workers */
struct completion *detach_completion; /* all workers detached */
@@ -297,6 +296,7 @@ static bool wq_numa_enabled; /* unbound NUMA affinity enabled */
static struct workqueue_attrs *wq_update_unbound_numa_attrs_buf;
static DEFINE_MUTEX(wq_pool_mutex); /* protects pools and workqueues list */
+static DEFINE_MUTEX(wq_pool_attach_mutex); /* protects worker attach/detach */
static DEFINE_SPINLOCK(wq_mayday_lock); /* protects wq->maydays list */
static DECLARE_WAIT_QUEUE_HEAD(wq_manager_wait); /* wait for manager to go away */
@@ -399,14 +399,14 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* @worker: iteration cursor
* @pool: worker_pool to iterate workers of
*
- * This must be called with @pool->attach_mutex.
+ * This must be called with wq_pool_attach_mutex.
*
* The if/else clause exists only for the lockdep assertion and can be
* ignored.
*/
#define for_each_pool_worker(worker, pool) \
list_for_each_entry((worker), &(pool)->workers, node) \
- if (({ lockdep_assert_held(&pool->attach_mutex); false; })) { } \
+ if (({ lockdep_assert_held(&wq_pool_attach_mutex); false; })) { } \
else
/**
@@ -1724,7 +1724,7 @@ static struct worker *alloc_worker(int node)
static void worker_attach_to_pool(struct worker *worker,
struct worker_pool *pool)
{
- mutex_lock(&pool->attach_mutex);
+ mutex_lock(&wq_pool_attach_mutex);
/*
* set_cpus_allowed_ptr() will fail if the cpumask doesn't have any
@@ -1733,16 +1733,16 @@ static void worker_attach_to_pool(struct worker *worker,
set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
/*
- * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains
- * stable across this function. See the comments above the
- * flag definition for details.
+ * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
+ * stable across this function. See the comments above the flag
+ * definition for details.
*/
if (pool->flags & POOL_DISASSOCIATED)
worker->flags |= WORKER_UNBOUND;
list_add_tail(&worker->node, &pool->workers);
- mutex_unlock(&pool->attach_mutex);
+ mutex_unlock(&wq_pool_attach_mutex);
}
/**
@@ -1759,11 +1759,11 @@ static void worker_detach_from_pool(struct worker *worker,
{
struct completion *detach_completion = NULL;
- mutex_lock(&pool->attach_mutex);
+ mutex_lock(&wq_pool_attach_mutex);
list_del(&worker->node);
if (list_empty(&pool->workers))
detach_completion = pool->detach_completion;
- mutex_unlock(&pool->attach_mutex);
+ mutex_unlock(&wq_pool_attach_mutex);
/* clear leftover flags without pool->lock after it is detached */
worker->flags &= ~(WORKER_UNBOUND | WORKER_REBOUND);
@@ -3271,7 +3271,6 @@ static int init_worker_pool(struct worker_pool *pool)
timer_setup(&pool->mayday_timer, pool_mayday_timeout, 0);
- mutex_init(&pool->attach_mutex);
INIT_LIST_HEAD(&pool->workers);
ida_init(&pool->worker_ida);
@@ -3354,10 +3353,10 @@ static void put_unbound_pool(struct worker_pool *pool)
WARN_ON(pool->nr_workers || pool->nr_idle);
spin_unlock_irq(&pool->lock);
- mutex_lock(&pool->attach_mutex);
+ mutex_lock(&wq_pool_attach_mutex);
if (!list_empty(&pool->workers))
pool->detach_completion = &detach_completion;
- mutex_unlock(&pool->attach_mutex);
+ mutex_unlock(&wq_pool_attach_mutex);
if (pool->detach_completion)
wait_for_completion(pool->detach_completion);
@@ -4600,7 +4599,7 @@ static void unbind_workers(int cpu)
struct worker *worker;
for_each_cpu_worker_pool(pool, cpu) {
- mutex_lock(&pool->attach_mutex);
+ mutex_lock(&wq_pool_attach_mutex);
spin_lock_irq(&pool->lock);
/*
@@ -4616,7 +4615,7 @@ static void unbind_workers(int cpu)
pool->flags |= POOL_DISASSOCIATED;
spin_unlock_irq(&pool->lock);
- mutex_unlock(&pool->attach_mutex);
+ mutex_unlock(&wq_pool_attach_mutex);
/*
* Call schedule() so that we cross rq->lock and thus can
@@ -4657,7 +4656,7 @@ static void rebind_workers(struct worker_pool *pool)
{
struct worker *worker;
- lockdep_assert_held(&pool->attach_mutex);
+ lockdep_assert_held(&wq_pool_attach_mutex);
/*
* Restore CPU affinity of all workers. As all idle workers should
@@ -4727,7 +4726,7 @@ static void restore_unbound_workers_cpumask(struct worker_pool *pool, int cpu)
static cpumask_t cpumask;
struct worker *worker;
- lockdep_assert_held(&pool->attach_mutex);
+ lockdep_assert_held(&wq_pool_attach_mutex);
/* is @cpu allowed for @pool? */
if (!cpumask_test_cpu(cpu, pool->attrs->cpumask))
@@ -4762,14 +4761,14 @@ int workqueue_online_cpu(unsigned int cpu)
mutex_lock(&wq_pool_mutex);
for_each_pool(pool, pi) {
- mutex_lock(&pool->attach_mutex);
+ mutex_lock(&wq_pool_attach_mutex);
if (pool->cpu == cpu)
rebind_workers(pool);
else if (pool->cpu < 0)
restore_unbound_workers_cpumask(pool, cpu);
- mutex_unlock(&pool->attach_mutex);
+ mutex_unlock(&wq_pool_attach_mutex);
}
/* update NUMA affinity of unbound workqueues */
--
2.9.5
^ permalink raw reply related
* [PATCH 1/6] proc: Don't allow empty /proc/PID/cmdline for user tasks
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm
Cc: linux-kernel, linux-api, kernel-team, csmall, Tejun Heo
In-Reply-To: <20180517043448.3152269-1-tj@kernel.org>
Kernel threads have empty /proc/PID/cmdline and some userland tools
including ps(1) and older versions of systemd use this to detect
kernel threads. However, any userland program can emulate the
behavior by making its argvs unavailable and trick the affected tools
into thinking that the task is a kernel thread. Linus's reproducer
follows.
#include <sys/prctl.h>
#include <sys/mman.h>
int main(void)
{
char empty[16384];
unsigned long ptr;
asm volatile("" :"=r" (ptr) : "0" (empty):"memory");
ptr = (ptr+4095) & ~4095;
munmap((void *)ptr, 32768);
sleep(1000);
return 0;
}
Compiling the above program into nullcmdline and running it on an
unpatche kernel shows the following behavior.
$ ./nullcmdline &
[1] 2382031
[devbig577 ~/tmp]$ hexdump -C /proc/2382031/comm
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 0a |nullcmdline.|
0000000c
$ hexdump -C /proc/2382031/cmdline
$ ps 2382031
PID TTY STAT TIME COMMAND
2382031 pts/2 S 0:00 [nullcmdline]
The empty cmdline makes ps(1) think that nullcmdline is a kernel
thread and put brackets around its name (comm), which is mostly a
nuisance but it's possible that this confusion can lead to more
harmful confusions.
This patch fixes the issue by making proc_pid_cmdline_read() never
return empty string for user tasks. If the result is empty for
whatever reason, comm string is returned. Even when the comm string
is empty, it still returns the null termnation character. On a
patched kernel, running the same command as above gives us.
$ ./nullcmdline &
[1] 2317
[test ~]# hexdump -C /proc/2317/comm
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 0a |nullcmdline.|
0000000c
$ hexdump -C /proc/2317/cmdline
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 00 |nullcmdline.|
0000000c
$ ps 2317
PID TTY STAT TIME COMMAND
2317 pts/0 S 0:00 nullcmdline
Note that cmdline is a dup of comm and ps(1) is no longer confused.
Signed-off-by: Tejun Heo <tj@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
---
fs/proc/base.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1b2ede6..2eee4d7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -224,9 +224,10 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
if (!tsk)
return -ESRCH;
mm = get_task_mm(tsk);
- put_task_struct(tsk);
- if (!mm)
- return 0;
+ if (!mm) {
+ rv = 0;
+ goto out_put_task;
+ }
/* Check if process spawned far enough to have cmdline. */
if (!mm->env_end) {
rv = 0;
@@ -367,8 +368,23 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
free_page((unsigned long)page);
out_mmput:
mmput(mm);
+out_put_task:
+ /*
+ * Some userland tools use empty cmdline to distinguish kthreads.
+ * Avoid empty cmdline for user tasks by returning tsk->comm with
+ * \0 termination when empty.
+ */
+ if (*pos == 0 && rv == 0 && !(tsk->flags & PF_KTHREAD)) {
+ char tcomm[TASK_COMM_LEN];
+
+ get_task_comm(tcomm, tsk);
+ rv = min(strlen(tcomm) + 1, count);
+ if (copy_to_user(buf, tsk->comm, rv))
+ rv = -EFAULT;
+ }
if (rv > 0)
*pos += rv;
+ put_task_struct(tsk);
return rv;
}
--
2.9.5
^ permalink raw reply related
* [PATCHSET] workqueue: Show the latest workqueue name in /proc/PID/{comm,stat,status}
From: Tejun Heo @ 2018-05-17 4:34 UTC (permalink / raw)
To: torvalds, jiangshanlai, akpm; +Cc: linux-kernel, linux-api, kernel-team, csmall
There can be a lot of workqueue workers and they all show up with the
cryptic kworker/* names making it difficult to understand which is
doing what and how they came to be.
# ps -ef | grep kworker
root 4 2 0 Feb25 ? 00:00:00 [kworker/0:0H]
root 6 2 0 Feb25 ? 00:00:00 [kworker/u112:0]
root 19 2 0 Feb25 ? 00:00:00 [kworker/1:0H]
root 25 2 0 Feb25 ? 00:00:00 [kworker/2:0H]
root 31 2 0 Feb25 ? 00:00:00 [kworker/3:0H]
...
This patchset makes workqueue workers report the latest workqueue it
was executing for through /proc/PID/{comm,stat,status}. The extra
information is appended to the kthread name with intervening '+' if
currently executing, otherwise '-'.
# cat /proc/25/comm
kworker/2:0-events_power_efficient
# cat /proc/25/stat
25 (kworker/2:0-events_power_efficient) I 2 0 0 0 -1 69238880 0 0...
# grep Name /proc/25/status
Name: kworker/2:0-events_power_efficient
For details on the design decisions, please refer to the following
thread.
http://lkml.kernel.org/r/20180516153939.GH2368884@devbig577.frc2.facebook.com
This patchset contains the following six patches.
0001-proc-Don-t-allow-empty-proc-PID-cmdline-for-user-tas.patch
0002-workqueue-Replace-pool-attach_mutex-with-global-wq_p.patch
0003-workqueue-Make-worker_attach-detach_pool-update-work.patch
0004-workqueue-Set-worker-desc-to-workqueue-name-by-defau.patch
0005-proc-Consolidate-task-comm-formatting-into-proc_task.patch
0006-workqueue-Show-the-latest-workqueue-name-in-proc-PID.patch
I'm applying the patches to wq/for-4.18. Please let me know if the
patchset need updates (the branch doesn't have any other changes
anyway). diffstat follows. Thanks.
fs/proc/array.c | 33 +++++++-----
fs/proc/base.c | 27 +++++++---
fs/proc/internal.h | 2
include/linux/workqueue.h | 1
kernel/workqueue.c | 117 ++++++++++++++++++++++++++++----------------
kernel/workqueue_internal.h | 3 -
6 files changed, 122 insertions(+), 61 deletions(-)
--
tejun
^ permalink raw reply
* [PATCH] proc: Don't allow empty /proc/PID/cmdline for user tasks
From: Tejun Heo @ 2018-05-17 1:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-kernel, linux-api, Andrew Morton, kernel-team,
Lennart Poettering
Kernel threads have empty /proc/PID/cmdline and some userland tools
including ps(1) and older versions of systemd use this to detect
kernel threads. However, any userland program can emulate the
behavior by making its argvs unavailable and trick the affected tools
into thinking that the task is a kernel thread. Linus's reproducer
follows.
#include <sys/prctl.h>
#include <sys/mman.h>
int main(void)
{
char empty[16384];
unsigned long ptr;
asm volatile("" :"=r" (ptr) : "0" (empty):"memory");
ptr = (ptr+4095) & ~4095;
munmap((void *)ptr, 32768);
sleep(1000);
return 0;
}
Compiling the above program into nullcmdline and running it on an
unpatche kernel shows the following behavior.
$ ./nullcmdline &
[1] 2382031
[devbig577 ~/tmp]$ hexdump -C /proc/2382031/comm
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 0a |nullcmdline.|
0000000c
$ hexdump -C /proc/2382031/cmdline
$ ps 2382031
PID TTY STAT TIME COMMAND
2382031 pts/2 S 0:00 [nullcmdline]
The empty cmdline makes ps(1) think that nullcmdline is a kernel
thread and put brackets around its name (comm), which is mostly a
nuisance but it's possible that this confusion can lead to more
harmful confusions.
This patch fixes the issue by making proc_pid_cmdline_read() never
return empty string for user tasks. If the result is empty for
whatever reason, comm string is returned. Even when the comm string
is empty, it still returns the null termnation character. On a
patched kernel, running the same command as above gives us.
$ ./nullcmdline &
[1] 2317
[test ~]# hexdump -C /proc/2317/comm
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 0a |nullcmdline.|
0000000c
$ hexdump -C /proc/2317/cmdline
00000000 6e 75 6c 6c 63 6d 64 6c 69 6e 65 00 |nullcmdline.|
0000000c
$ ps 2317
PID TTY STAT TIME COMMAND
2317 pts/0 S 0:00 nullcmdline
Note that cmdline is a dup of comm and ps(1) is no longer confused.
Signed-off-by: Tejun Heo <tj@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
---
Hello,
Linus, this patch is somewhat different from the rest of workqueue
changes and it could make sense to apply separately, so please feel
free to apply directly. If you want it to be routed together with the
other workqueue changes, please let me know.
Thanks.
fs/proc/base.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -224,9 +224,10 @@ static ssize_t proc_pid_cmdline_read(str
if (!tsk)
return -ESRCH;
mm = get_task_mm(tsk);
- put_task_struct(tsk);
- if (!mm)
- return 0;
+ if (!mm) {
+ rv = 0;
+ goto out_put_task;
+ }
/* Check if process spawned far enough to have cmdline. */
if (!mm->env_end) {
rv = 0;
@@ -367,8 +368,23 @@ out_free_page:
free_page((unsigned long)page);
out_mmput:
mmput(mm);
+out_put_task:
+ /*
+ * Some userland tools use empty cmdline to distinguish kthreads.
+ * Avoid empty cmdline for user tasks by returning tsk->comm with
+ * \0 termination when empty.
+ */
+ if (*pos == 0 && rv == 0 && !(tsk->flags & PF_KTHREAD)) {
+ char tcomm[TASK_COMM_LEN];
+
+ get_task_comm(tcomm, tsk);
+ rv = min(strlen(tcomm) + 1, count);
+ if (copy_to_user(buf, tsk->comm, rv))
+ rv = -EFAULT;
+ }
if (rv > 0)
*pos += rv;
+ put_task_struct(tsk);
return rv;
}
^ permalink raw reply
* Re: [PATCH 07/14] powerpc: Add support for restartable sequences
From: Boqun Feng @ 2018-05-17 1:19 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E. McKenney, 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
In-Reply-To: <112970629.1913.1526501596485.JavaMail.zimbra@efficios.com>
[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]
On Wed, May 16, 2018 at 04:13:16PM -0400, Mathieu Desnoyers wrote:
> ----- On May 16, 2018, at 12:18 PM, Peter Zijlstra peterz@infradead.org wrote:
>
> > On Mon, Apr 30, 2018 at 06:44:26PM -0400, Mathieu Desnoyers wrote:
> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> >> index c32a181a7cbb..ed21a777e8c6 100644
> >> --- a/arch/powerpc/Kconfig
> >> +++ b/arch/powerpc/Kconfig
> >> @@ -223,6 +223,7 @@ config PPC
> >> select HAVE_SYSCALL_TRACEPOINTS
> >> select HAVE_VIRT_CPU_ACCOUNTING
> >> select HAVE_IRQ_TIME_ACCOUNTING
> >> + select HAVE_RSEQ
> >> select IRQ_DOMAIN
> >> select IRQ_FORCED_THREADING
> >> select MODULES_USE_ELF_RELA
> >> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> >> index 61db86ecd318..d3bb3aaaf5ac 100644
> >> --- a/arch/powerpc/kernel/signal.c
> >> +++ b/arch/powerpc/kernel/signal.c
> >> @@ -133,6 +133,8 @@ static void do_signal(struct task_struct *tsk)
> >> /* Re-enable the breakpoints for the signal stack */
> >> thread_change_pc(tsk, tsk->thread.regs);
> >>
> >> + rseq_signal_deliver(tsk->thread.regs);
> >> +
> >> if (is32) {
> >> if (ksig.ka.sa.sa_flags & SA_SIGINFO)
> >> ret = handle_rt_signal32(&ksig, oldset, tsk);
> >> @@ -164,6 +166,7 @@ void do_notify_resume(struct pt_regs *regs, unsigned long
> >> thread_info_flags)
> >> if (thread_info_flags & _TIF_NOTIFY_RESUME) {
> >> clear_thread_flag(TIF_NOTIFY_RESUME);
> >> tracehook_notify_resume(regs);
> >> + rseq_handle_notify_resume(regs);
> >> }
> >>
> >> user_enter();
> >
> > Again no rseq_syscall().
>
> Same question for PowerPC as for ARM:
>
> Considering that rseq_syscall is implemented as follows:
>
> +void rseq_syscall(struct pt_regs *regs)
> +{
> + unsigned long ip = instruction_pointer(regs);
> + struct task_struct *t = current;
> + struct rseq_cs rseq_cs;
> +
> + if (!t->rseq)
> + return;
> + if (!access_ok(VERIFY_READ, t->rseq, sizeof(*t->rseq)) ||
> + rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
> + force_sig(SIGSEGV, t);
> +}
>
> and that x86 calls it from syscall_return_slowpath() (which AFAIU is
> now used in the fast-path since KPTI), I wonder where we should call
So we actually detect this after the syscall takes effect, right? I
wonder whether this could be problematic, because "disallowing syscall"
in rseq areas may means the syscall won't take effect to some people, I
guess?
> this on PowerPC ? I was under the impression that PowerPC return to
> userspace fast-path was not calling C code unless work flags were set,
> but I might be wrong.
>
I think you're right. So we have to introduce callsite to rseq_syscall()
in syscall path, something like:
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 51695608c68b..a25734a96640 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -222,6 +222,9 @@ system_call_exit:
mtmsrd r11,1
#endif /* CONFIG_PPC_BOOK3E */
+ addi r3,r1,STACK_FRAME_OVERHEAD
+ bl rseq_syscall
+
ld r9,TI_FLAGS(r12)
li r11,-MAX_ERRNO
andi. r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
But I think it's important for us to first decide where (before or after
the syscall) we do the detection.
Regards,
Boqun
> Thoughts ?
>
> Thanks!
>
> Mathieu
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related
* Re: [RFC PATCH] UAPI: Document auxvec AT_* namespace policy and note reservations
From: Rich Felker @ 2018-05-16 23:21 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Dave Martin, linux-kernel, x86, linux-arch, linux-api,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Russell King,
Catalin Marinas, Will Deacon, Tony Luck, Fenghua Yu, Michal Simek,
Ralf Baechle, James Hogan, Greentime Hu, Vincent Chen,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Palmer Dabbelt <pal>
In-Reply-To: <3e291fda-b27b-7002-9374-23196f18a38f@zytor.com>
On Wed, May 16, 2018 at 04:09:29PM -0700, H. Peter Anvin wrote:
> On 05/16/18 08:49, Dave Martin wrote:
> >
> > Since <linux/auxvec.h> only contains #defines, it may be enough for arch
> > <asm/auxvec.h> headers to include <linux/auxvec.h>.
> >
>
> <asm/auxvec.h> doesn't seem to have any reason to exist at all. If
> anyone includes it now, they are Doing It Wrong[TM] since
> <linux/auxvec.h> includes <asm/auxvec.h>.
Sounds good. BTW there probably does need to be a check for
kernel-internal use of #ifdef AT_* etc. I saw some instances in
arch/um and a few other weird places. They might not matter but there
might be code that's indirectly enabled only for certain archs via the
AT_* macros.
Rich
^ permalink raw reply
* Re: [RFC PATCH] UAPI: Document auxvec AT_* namespace policy and note reservations
From: H. Peter Anvin @ 2018-05-16 23:09 UTC (permalink / raw)
To: Dave Martin, Rich Felker
Cc: linux-kernel, x86, linux-arch, linux-api, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Tony Luck, Fenghua Yu, Michal Simek, Ralf Baechle,
James Hogan, Greentime Hu, Vincent Chen, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Palmer Dabbelt, Albert
In-Reply-To: <20180516154957.GT7753@e103592.cambridge.arm.com>
On 05/16/18 08:49, Dave Martin wrote:
>
> Since <linux/auxvec.h> only contains #defines, it may be enough for arch
> <asm/auxvec.h> headers to include <linux/auxvec.h>.
>
<asm/auxvec.h> doesn't seem to have any reason to exist at all. If
anyone includes it now, they are Doing It Wrong[TM] since
<linux/auxvec.h> includes <asm/auxvec.h>.
-hpa
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Ram Pai @ 2018-05-16 21:07 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, linux-arch, Linux-MM, Linux API, X86 ML,
Dave Hansen, linux-x86_64, linuxppc-dev
In-Reply-To: <CALCETrVQs=ix-w9_MLJWikzmBG-e2Fzg61TrZLNVv5R3XFOs=g@mail.gmail.com>
On Wed, May 16, 2018 at 01:37:46PM -0700, Andy Lutomirski wrote:
> On Wed, May 16, 2018 at 1:35 PM Ram Pai <linuxram@us.ibm.com> wrote:
>
> > On Tue, May 08, 2018 at 02:40:46PM +0200, Florian Weimer wrote:
> > > On 05/08/2018 04:49 AM, Andy Lutomirski wrote:
> > > >On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com>
> wrote:
> > > >
> > > >>On 05/03/2018 06:05 AM, Andy Lutomirski wrote:
> > > >>>On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote:
> > > >>>
> > > >>>>On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote:
> > > >>>>>
> > > >>>>>>If I recall correctly, the POWER maintainer did express a strong
> > > >>>desire
> > > >>>>>>back then for (what is, I believe) their current semantics, which
> my
> > > >>>>>>PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too.
> > > >>>>>
> > > >>>>>Ram, I really really don't like the POWER semantics. Can you give
> > > >some
> > > >>>>>justification for them? Does POWER at least have an atomic way for
> > > >>>>>userspace to modify just the key it wants to modify or, even
> better,
> > > >>>>>special load and store instructions to use alternate keys?
> > > >>>
> > > >>>>I wouldn't call it POWER semantics. The way I implemented it on
> power
> > > >>>>lead to the semantics, given that nothing was explicitly stated
> > > >>>>about how the semantics should work within a signal handler.
> > > >>>
> > > >>>I think that this is further evidence that we should introduce a new
> > > >>>pkey_alloc() mode and deprecate the old. To the extent possible,
> this
> > > >>>thing should work the same way on x86 and POWER.
> > > >
> > > >>Do you propose to change POWER or to change x86?
> > > >
> > > >Sorry for being slow to reply. I propose to introduce a new
> > > >PKEY_ALLOC_something variant on x86 and POWER and to make the behavior
> > > >match on both.
> > >
> > > So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the
> > > existing (different) behavior without the flag?
> > >
> > > Ram, would you be okay with that? Could you give me a hand if
> > > necessary? (I assume we have silicon in-house because it's a
> > > long-standing feature of the POWER platform which was simply dormant
> > > on Linux until now.)
>
> > Yes. I can help you with that.
>
> > So let me see if I understand the overall idea.
>
> > Application can allocate new keys through a new syscall
> > sys_pkey_alloc_1(flags, init_val, sig_init_val)
>
> > 'sig_init_val' is the permission-state of the key in signal context.
>
> > The kernel will set the permission of each keys to their
> > corresponding values when entering the signal handler and revert
> > on return from the signal handler.
>
> > just like init_val, sig_init_val also percolates to children threads.
>
>
> I was imagining it would be just pkey_alloc(SOME_NEW_FLAG, init_val); and
> the init val would be used for the current thread and for signal handlers.
what would change the key-permission-values enforced in signal-handler
context? Or can it never be changed, ones set through sys_pkey_alloc()?
I suppose key-permission-values change done in non-signal-handler context,
will not apply to those in signal-handler context.
Can the signal handler change the key-permission-values from the
signal-handler context?
RP
^ 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;
as well as URLs for NNTP newsgroup(s).