* [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
@ 2012-03-07 2:26 David Rientjes
2012-03-08 20:02 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: David Rientjes @ 2012-03-07 2:26 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dave Jones, linux-kernel, linux-mm
Add the thread name and pid of the application that is allocating shm
segments with MAP_HUGETLB without being a part of
/proc/sys/vm/hugetlb_shm_group or having CAP_IPC_LOCK.
This identifies the application so it may be fixed by avoiding using the
deprecated exception (see Documentation/feature-removal-schedule.txt).
Signed-off-by: David Rientjes <rientjes@google.com>
---
fs/hugetlbfs/inode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -946,7 +946,11 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
*user = current_user();
if (user_shm_lock(size, *user)) {
- printk_once(KERN_WARNING "Using mlock ulimits for SHM_HUGETLB is deprecated\n");
+ task_lock(current);
+ printk_once(KERN_WARNING
+ "%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
+ current->comm, current->pid);
+ task_unlock(current);
} else {
*user = NULL;
return ERR_PTR(-EPERM);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
2012-03-07 2:26 [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning David Rientjes
@ 2012-03-08 20:02 ` Andrew Morton
2012-03-08 21:37 ` David Rientjes
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-03-08 20:02 UTC (permalink / raw)
To: David Rientjes; +Cc: Dave Jones, linux-kernel, linux-mm
On Tue, 6 Mar 2012 18:26:11 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:
> Add the thread name and pid of the application that is allocating shm
> segments with MAP_HUGETLB without being a part of
> /proc/sys/vm/hugetlb_shm_group or having CAP_IPC_LOCK.
>
> This identifies the application so it may be fixed by avoiding using the
> deprecated exception (see Documentation/feature-removal-schedule.txt).
>
> ...
>
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -946,7 +946,11 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
> if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
> *user = current_user();
> if (user_shm_lock(size, *user)) {
> - printk_once(KERN_WARNING "Using mlock ulimits for SHM_HUGETLB is deprecated\n");
> + task_lock(current);
> + printk_once(KERN_WARNING
> + "%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
> + current->comm, current->pid);
> + task_unlock(current);
I assume the task_lock() is there to protect current->comm. If so, it
is unneeded - we're protecting against prctl(PR_SET_NAME), and
PR_SET_NAME only operates on current, and we know this task isn't
currently running PR_SET_NAME.
If there's a way for another task to alter this task's ->comm then we
_do_ need locking. But there isn't a way, I hope.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
2012-03-08 20:02 ` Andrew Morton
@ 2012-03-08 21:37 ` David Rientjes
2012-03-08 21:56 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: David Rientjes @ 2012-03-08 21:37 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dave Jones, linux-kernel, linux-mm
On Thu, 8 Mar 2012, Andrew Morton wrote:
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -946,7 +946,11 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
> > if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
> > *user = current_user();
> > if (user_shm_lock(size, *user)) {
> > - printk_once(KERN_WARNING "Using mlock ulimits for SHM_HUGETLB is deprecated\n");
> > + task_lock(current);
> > + printk_once(KERN_WARNING
> > + "%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
> > + current->comm, current->pid);
> > + task_unlock(current);
>
> I assume the task_lock() is there to protect current->comm.
Yup.
> If so, it
> is unneeded - we're protecting against prctl(PR_SET_NAME), and
> PR_SET_NAME only operates on current, and we know this task isn't
> currently running PR_SET_NAME.
>
> If there's a way for another task to alter this task's ->comm then we
> _do_ need locking. But there isn't a way, I hope.
>
I wish there wasn't as well, it would prevent a lot of the currently buggy
reads to current->comm and allow us to avoid so many otherwise pointless
task_lock()s.
This protects against /proc/pid/comm, which is writable by threads in the
same thread group. We have a get_task_comm() that does the task_lock()
internally but requires a TASK_COMM_LEN buffer in the calling code. It's
just easier for the calling code to the task_lock() itself for a tiny
little printk().
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
2012-03-08 21:37 ` David Rientjes
@ 2012-03-08 21:56 ` Andrew Morton
2012-03-08 22:08 ` David Rientjes
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-03-08 21:56 UTC (permalink / raw)
To: David Rientjes; +Cc: Dave Jones, linux-kernel, linux-mm
On Thu, 8 Mar 2012 13:37:57 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:
> On Thu, 8 Mar 2012, Andrew Morton wrote:
>
> > > --- a/fs/hugetlbfs/inode.c
> > > +++ b/fs/hugetlbfs/inode.c
> > > @@ -946,7 +946,11 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
> > > if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
> > > *user = current_user();
> > > if (user_shm_lock(size, *user)) {
> > > - printk_once(KERN_WARNING "Using mlock ulimits for SHM_HUGETLB is deprecated\n");
> > > + task_lock(current);
> > > + printk_once(KERN_WARNING
> > > + "%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
> > > + current->comm, current->pid);
> > > + task_unlock(current);
> >
> > I assume the task_lock() is there to protect current->comm.
>
> Yup.
>
> > If so, it
> > is unneeded - we're protecting against prctl(PR_SET_NAME), and
> > PR_SET_NAME only operates on current, and we know this task isn't
> > currently running PR_SET_NAME.
> >
> > If there's a way for another task to alter this task's ->comm then we
> > _do_ need locking. But there isn't a way, I hope.
> >
>
> I wish there wasn't as well, it would prevent a lot of the currently buggy
> reads to current->comm and allow us to avoid so many otherwise pointless
> task_lock()s.
>
> This protects against /proc/pid/comm, which is writable by threads in the
> same thread group.
Oh crap.
> We have a get_task_comm() that does the task_lock()
> internally but requires a TASK_COMM_LEN buffer in the calling code. It's
> just easier for the calling code to the task_lock() itself for a tiny
> little printk().
Well for a tiny little printk we could just omit the locking? The
printk() won't oops and once in a million years one person will see a
garbled comm[] string?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
2012-03-08 21:56 ` Andrew Morton
@ 2012-03-08 22:08 ` David Rientjes
2012-03-08 22:23 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: David Rientjes @ 2012-03-08 22:08 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dave Jones, linux-kernel, linux-mm
On Thu, 8 Mar 2012, Andrew Morton wrote:
> > We have a get_task_comm() that does the task_lock()
> > internally but requires a TASK_COMM_LEN buffer in the calling code. It's
> > just easier for the calling code to the task_lock() itself for a tiny
> > little printk().
>
> Well for a tiny little printk we could just omit the locking? The
> printk() won't oops and once in a million years one person will see a
> garbled comm[] string?
>
Sure, but task_lock() shouldn't be highly contended when the thread isn't
forking or exiting (everything else is attaching/detaching from a cgroup
or testing a mempolicy). I've always added it (like in the oom killer for
the same reason) just because the race exists. Taking it for every thread
on the system for one call to the oom killer has never slowed it down.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning
2012-03-08 22:08 ` David Rientjes
@ 2012-03-08 22:23 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2012-03-08 22:23 UTC (permalink / raw)
To: David Rientjes; +Cc: Dave Jones, linux-kernel, linux-mm
On Thu, 8 Mar 2012 14:08:30 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:
> On Thu, 8 Mar 2012, Andrew Morton wrote:
>
> > > We have a get_task_comm() that does the task_lock()
> > > internally but requires a TASK_COMM_LEN buffer in the calling code. It's
> > > just easier for the calling code to the task_lock() itself for a tiny
> > > little printk().
> >
> > Well for a tiny little printk we could just omit the locking? The
> > printk() won't oops and once in a million years one person will see a
> > garbled comm[] string?
> >
>
> Sure, but task_lock() shouldn't be highly contended when the thread isn't
> forking or exiting (everything else is attaching/detaching from a cgroup
> or testing a mempolicy). I've always added it (like in the oom killer for
> the same reason) just because the race exists. Taking it for every thread
> on the system for one call to the oom killer has never slowed it down.
I wasn't concerned about the performance side of things - just that
it's such a pain over such a silly thing.
btw, if the code had done
printk_once(..., get_task_comm(...), ...)
the task_lock() would have been performed just a single time, rather
than every time.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-08 22:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-07 2:26 [patch] mm, hugetlb: add thread name and pid to SHM_HUGETLB mlock rlimit warning David Rientjes
2012-03-08 20:02 ` Andrew Morton
2012-03-08 21:37 ` David Rientjes
2012-03-08 21:56 ` Andrew Morton
2012-03-08 22:08 ` David Rientjes
2012-03-08 22:23 ` Andrew Morton
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).