Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rss_stat: Add support to detect RSS updates of external mm
@ 2019-11-06  2:44 Joel Fernandes (Google)
  2019-11-06  8:00 ` Sergey Senozhatsky
  2019-11-06  8:59 ` Petr Mladek
  0 siblings, 2 replies; 5+ messages in thread
From: Joel Fernandes (Google) @ 2019-11-06  2:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Ioannis Ilkos, minchan, primiano, fmayer, hjd, joaodias, joelaf,
	lalitm, rslawik, sspatil, timmurray, Andrew Morton,
	Andy Shevchenko, Changbin Du, Ingo Molnar, Joe Perches,
	Kees Cook, linux-mm, Michal Hocko, Petr Mladek,
	Rafael J. Wysocki, Sakari Ailus, Sergey Senozhatsky,
	Stephen Rothwell, Steven Rostedt

When a process updates the RSS of a different process, the rss_stat
tracepoint appears in the context of the process doing the update. This
can confuse userspace that the RSS of process doing the update is
updated, while in reality a different process's RSS was updated.

This issue happens in reclaim paths such as with direct reclaim or
background reclaim.

This patch adds more information to the tracepoint about whether the mm
being updated belongs to the current process's context (curr field). We
also include a hash of the mm pointer so that the process who the mm
belongs to can be uniquely identified (mm_id field).

Also vsprintf.c is refactored a bit to allow reuse of hashing code.

Reported-by: Ioannis Ilkos <ilkos@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
Based on top of the commit in linux-next:
8342d836dc7c ("mm: emit tracepoint when RSS changes")

Google Bug: 140711541

Cc: minchan@google.com
Cc: primiano@google.com
Cc: ilkos@google.com
Cc: fmayer@google.com
Cc: hjd@google.com
Cc: joaodias@google.com
Cc: joelaf@google.com
Cc: lalitm@google.com
Cc: rslawik@google.com
Cc: sspatil@google.com
Cc: timmurray@google.com

 include/linux/mm.h          |  8 ++++----
 include/linux/string.h      |  2 ++
 include/trace/events/kmem.h | 32 +++++++++++++++++++++++++++++---
 lib/vsprintf.c              | 36 +++++++++++++++++++++++++-----------
 mm/memory.c                 |  4 ++--
 5 files changed, 62 insertions(+), 20 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 31d8cfb3d988..bfbe65ccffa3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1643,27 +1643,27 @@ static inline unsigned long get_mm_counter(struct mm_struct *mm, int member)
 	return (unsigned long)val;
 }
 
-void mm_trace_rss_stat(int member, long count);
+void mm_trace_rss_stat(struct mm_struct *mm, int member, long count);
 
 static inline void add_mm_counter(struct mm_struct *mm, int member, long value)
 {
 	long count = atomic_long_add_return(value, &mm->rss_stat.count[member]);
 
-	mm_trace_rss_stat(member, count);
+	mm_trace_rss_stat(mm, member, count);
 }
 
 static inline void inc_mm_counter(struct mm_struct *mm, int member)
 {
 	long count = atomic_long_inc_return(&mm->rss_stat.count[member]);
 
-	mm_trace_rss_stat(member, count);
+	mm_trace_rss_stat(mm, member, count);
 }
 
 static inline void dec_mm_counter(struct mm_struct *mm, int member)
 {
 	long count = atomic_long_dec_return(&mm->rss_stat.count[member]);
 
-	mm_trace_rss_stat(member, count);
+	mm_trace_rss_stat(mm, member, count);
 }
 
 /* Optimized variant when page is already known not to be PageAnon */
diff --git a/include/linux/string.h b/include/linux/string.h
index f516cec5277c..6b0e950701d0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -261,6 +261,8 @@ int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4);
 extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
 				       const void *from, size_t available);
 
+int ptr_to_hashval(const void *ptr, unsigned long *hashval_out);
+
 /**
  * strstarts - does @str start with @prefix?
  * @str: string to examine
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index 5a0666bfcf85..ad7e642bd497 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -316,24 +316,50 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->change_ownership)
 );
 
+/*
+ * Required for uniquely and securely identifying mm in rss_stat tracepoint.
+ */
+#ifndef __PTR_TO_HASHVAL
+static unsigned int __maybe_unused mm_ptr_to_hash(const void *ptr)
+{
+	int ret;
+	unsigned long hashval;
+
+	ret = ptr_to_hashval(ptr, &hashval);
+	if (ret)
+		return 0;
+
+	/* The hashed value is only 32-bit */
+	return (unsigned int)hashval;
+}
+#define __PTR_TO_HASHVAL
+#endif
+
 TRACE_EVENT(rss_stat,
 
-	TP_PROTO(int member,
+	TP_PROTO(struct mm_struct *mm,
+		int member,
 		long count),
 
-	TP_ARGS(member, count),
+	TP_ARGS(mm, member, count),
 
 	TP_STRUCT__entry(
+		__field(unsigned int, mm_id)
+		__field(unsigned int, curr)
 		__field(int, member)
 		__field(long, size)
 	),
 
 	TP_fast_assign(
+		__entry->mm_id = mm_ptr_to_hash(mm);
+		__entry->curr = !!(current->mm == mm);
 		__entry->member = member;
 		__entry->size = (count << PAGE_SHIFT);
 	),
 
-	TP_printk("member=%d size=%ldB",
+	TP_printk("mm_id=%u curr=%d member=%d size=%ldB",
+		__entry->mm_id,
+		__entry->curr,
 		__entry->member,
 		__entry->size)
 	);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dee8fc467fcf..401baaac1813 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
 early_initcall(initialize_ptr_random);
 
 /* Maps a pointer to a 32 bit unique identifier. */
+int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
+{
+	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
+	unsigned long hashval;
+
+	if (static_branch_unlikely(&not_filled_random_ptr_key))
+		return -EAGAIN;
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+	/*
+	 * Mask off the first 32 bits, this makes explicit that we have
+	 * modified the address (and 32 bits is plenty for a unique ID).
+	 */
+	hashval = hashval & 0xffffffff;
+#else
+	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+	*hashval_out = hashval;
+	return 0;
+}
+
 static char *ptr_to_id(char *buf, char *end, const void *ptr,
 		       struct printf_spec spec)
 {
 	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
 	unsigned long hashval;
+	int ret;
 
 	/* When debugging early boot use non-cryptographically secure hash. */
 	if (unlikely(debug_boot_weak_hash)) {
@@ -773,22 +796,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 		return pointer_string(buf, end, (const void *)hashval, spec);
 	}
 
-	if (static_branch_unlikely(&not_filled_random_ptr_key)) {
+	ret = ptr_to_hashval(ptr, &hashval);
+	if (ret) {
 		spec.field_width = 2 * sizeof(ptr);
 		/* string length must be less than default_width */
 		return error_string(buf, end, str, spec);
 	}
 
-#ifdef CONFIG_64BIT
-	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
-	/*
-	 * Mask off the first 32 bits, this makes explicit that we have
-	 * modified the address (and 32 bits is plenty for a unique ID).
-	 */
-	hashval = hashval & 0xffffffff;
-#else
-	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
-#endif
 	return pointer_string(buf, end, (const void *)hashval, spec);
 }
 
diff --git a/mm/memory.c b/mm/memory.c
index 7596d625ebd1..d3c9784e6dc1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -154,9 +154,9 @@ static int __init init_zero_pfn(void)
 }
 core_initcall(init_zero_pfn);
 
-void mm_trace_rss_stat(int member, long count)
+void mm_trace_rss_stat(struct mm_struct *mm, int member, long count)
 {
-	trace_rss_stat(member, count);
+	trace_rss_stat(mm, member, count);
 }
 
 #if defined(SPLIT_RSS_COUNTING)
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm
  2019-11-06  2:44 [PATCH] rss_stat: Add support to detect RSS updates of external mm Joel Fernandes (Google)
@ 2019-11-06  8:00 ` Sergey Senozhatsky
  2019-11-06  8:59 ` Petr Mladek
  1 sibling, 0 replies; 5+ messages in thread
From: Sergey Senozhatsky @ 2019-11-06  8:00 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Ioannis Ilkos, minchan, primiano, fmayer, hjd,
	joaodias, joelaf, lalitm, rslawik, sspatil, timmurray,
	Andrew Morton, Andy Shevchenko, Changbin Du, Ingo Molnar,
	Joe Perches, Kees Cook, linux-mm, Michal Hocko, Petr Mladek,
	Rafael J. Wysocki, Sakari Ailus, Sergey Senozhatsky,
	Stephen Rothwell, Steven Rostedt

On (19/11/05 21:44), Joel Fernandes (Google) wrote:
[..]
> +++ b/lib/vsprintf.c
> @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
>  early_initcall(initialize_ptr_random);
>  
>  /* Maps a pointer to a 32 bit unique identifier. */
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> +{
> +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> +	unsigned long hashval;
> +
> +	if (static_branch_unlikely(&not_filled_random_ptr_key))
> +		return -EAGAIN;
> +
> +#ifdef CONFIG_64BIT
> +	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> +	/*
> +	 * Mask off the first 32 bits, this makes explicit that we have
> +	 * modified the address (and 32 bits is plenty for a unique ID).
> +	 */
> +	hashval = hashval & 0xffffffff;
> +#else
> +	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> +	*hashval_out = hashval;
> +	return 0;
> +}

Maybe we want to move this thing to more generic code - lib/siphash.c -
since it's going to have several users.

	-ss


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm
  2019-11-06  2:44 [PATCH] rss_stat: Add support to detect RSS updates of external mm Joel Fernandes (Google)
  2019-11-06  8:00 ` Sergey Senozhatsky
@ 2019-11-06  8:59 ` Petr Mladek
  2019-11-07 18:07   ` Joel Fernandes
  1 sibling, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2019-11-06  8:59 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Ioannis Ilkos, minchan, primiano, fmayer, hjd,
	joaodias, joelaf, lalitm, rslawik, sspatil, timmurray,
	Andrew Morton, Andy Shevchenko, Changbin Du, Ingo Molnar,
	Joe Perches, Kees Cook, linux-mm, Michal Hocko,
	Rafael J. Wysocki, Sakari Ailus, Sergey Senozhatsky,
	Stephen Rothwell, Steven Rostedt

On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> Also vsprintf.c is refactored a bit to allow reuse of hashing code.

I agree with Sergey that it would make sense to move this outside
vsprintf.c.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index dee8fc467fcf..401baaac1813 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
>  early_initcall(initialize_ptr_random);
>  
>  /* Maps a pointer to a 32 bit unique identifier. */
> +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> +{
> +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";

str is unused.

> +	unsigned long hashval;

IMHO, this local variable unnecessarily complicates the code.
I would use the pointer directly and let compiler to optimize.

> +	if (static_branch_unlikely(&not_filled_random_ptr_key))
> +		return -EAGAIN;
> +
> +#ifdef CONFIG_64BIT
> +	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> +	/*
> +	 * Mask off the first 32 bits, this makes explicit that we have
> +	 * modified the address (and 32 bits is plenty for a unique ID).
> +	 */
> +	hashval = hashval & 0xffffffff;
> +#else
> +	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> +	*hashval_out = hashval;
> +	return 0;
> +}

Best Regards,
Petr


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm
  2019-11-06  8:59 ` Petr Mladek
@ 2019-11-07 18:07   ` Joel Fernandes
  2019-11-08  8:47     ` Petr Mladek
  0 siblings, 1 reply; 5+ messages in thread
From: Joel Fernandes @ 2019-11-07 18:07 UTC (permalink / raw)
  To: Petr Mladek
  Cc: linux-kernel, Ioannis Ilkos, minchan, primiano, fmayer, hjd,
	joaodias, lalitm, rslawik, sspatil, timmurray, Andrew Morton,
	Andy Shevchenko, Changbin Du, Ingo Molnar, Joe Perches,
	Kees Cook, linux-mm, Michal Hocko, Rafael J. Wysocki,
	Sakari Ailus, Sergey Senozhatsky, Stephen Rothwell,
	Steven Rostedt

On Wed, Nov 06, 2019 at 09:59:59AM +0100, Petr Mladek wrote:
> On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> > Also vsprintf.c is refactored a bit to allow reuse of hashing code.
> 
> I agree with Sergey that it would make sense to move this outside
> vsprintf.c.

I am of the opinion that its Ok to have it this way and I am not sure if
another translation unit is worth it just for this. If we have more users,
then yes we can consider splitting into its own translation unit at that
time.

If Andrew Morton objects, then I'll do it since the intention was for this
patch to go through his tree and I believe he is Ok with it this way.

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index dee8fc467fcf..401baaac1813 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> >  early_initcall(initialize_ptr_random);
> >  
> >  /* Maps a pointer to a 32 bit unique identifier. */
> > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > +{
> > +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> 
> str is unused.

I believe Andrew has already fixed this in his tree.

linux-next has local variable removed now:
7422993b4f8e ("rss_stat-add-support-to-detect-rss-updates-of-external-mm-fix")

> > +	unsigned long hashval;
> 
> IMHO, this local variable unnecessarily complicates the code.
> I would use the pointer directly and let compiler to optimize.

Agreed.

thanks,

 - Joel


> > +	if (static_branch_unlikely(&not_filled_random_ptr_key))
> > +		return -EAGAIN;
> > +
> > +#ifdef CONFIG_64BIT
> > +	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> > +	/*
> > +	 * Mask off the first 32 bits, this makes explicit that we have
> > +	 * modified the address (and 32 bits is plenty for a unique ID).
> > +	 */
> > +	hashval = hashval & 0xffffffff;
> > +#else
> > +	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> > +#endif
> > +	*hashval_out = hashval;
> > +	return 0;
> > +}
> 
> Best Regards,
> Petr


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rss_stat: Add support to detect RSS updates of external mm
  2019-11-07 18:07   ` Joel Fernandes
@ 2019-11-08  8:47     ` Petr Mladek
  0 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2019-11-08  8:47 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Ioannis Ilkos, minchan, primiano, fmayer, hjd,
	joaodias, lalitm, rslawik, sspatil, timmurray, Andrew Morton,
	Andy Shevchenko, Changbin Du, Ingo Molnar, Joe Perches,
	Kees Cook, linux-mm, Michal Hocko, Rafael J. Wysocki,
	Sakari Ailus, Sergey Senozhatsky, Stephen Rothwell,
	Steven Rostedt

On Thu 2019-11-07 13:07:51, Joel Fernandes wrote:
> On Wed, Nov 06, 2019 at 09:59:59AM +0100, Petr Mladek wrote:
> > On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> > > Also vsprintf.c is refactored a bit to allow reuse of hashing code.
> > 
> > I agree with Sergey that it would make sense to move this outside
> > vsprintf.c.
> 
> I am of the opinion that its Ok to have it this way and I am not sure if
> another translation unit is worth it just for this. If we have more users,
> then yes we can consider splitting into its own translation unit at that
> time.

Fair enough. I still think that it would make sense to move the code
but I do not want to block this patch because of this.

My view is that vsprintf.c is huge and the hashing-related code
is lost there. It consists of more pieces, early_param, static key,
workqueue work.

> If Andrew Morton objects, then I'll do it since the intention was for this
> patch to go through his tree and I believe he is Ok with it this way.
> 
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index dee8fc467fcf..401baaac1813 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> > >  early_initcall(initialize_ptr_random);
> > >  
> > >  /* Maps a pointer to a 32 bit unique identifier. */
> > > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > > +{
> > > +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> > 
> > str is unused.
> 
> I believe Andrew has already fixed this in his tree.
> 
> linux-next has local variable removed now:
> 7422993b4f8e ("rss_stat-add-support-to-detect-rss-updates-of-external-mm-fix")

I see. With this fix, feel free to add:

Acked-by: Petr Mladek <pmladek@suse.com> # lib/vsprintf.c

The clean up could be done by a followup patches.

Best Regards,
Petr


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06  2:44 [PATCH] rss_stat: Add support to detect RSS updates of external mm Joel Fernandes (Google)
2019-11-06  8:00 ` Sergey Senozhatsky
2019-11-06  8:59 ` Petr Mladek
2019-11-07 18:07   ` Joel Fernandes
2019-11-08  8:47     ` Petr Mladek

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git