diff mbox series

[11/12] fscache: Fix fscache_cookie_put() to not deref after dec

Message ID 162431203107.2908479.3259582550347000088.stgit@warthog.procyon.org.uk
State New
Headers show
Series fscache: Some prep work for fscache rewrite | expand

Commit Message

David Howells June 21, 2021, 9:47 p.m. UTC
fscache_cookie_put() accesses the cookie it has just put inside the
tracepoint that monitors the change - but this is something it's not
allowed to do if we didn't reduce the count to zero.

Fix this by dropping most of those values from the tracepoint and grabbing
the cookie debug ID before doing the dec.

Also take the opportunity to switch over the usage and where arguments on
the tracepoint to put the reason last.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/fscache/cookie.c            |   10 ++++++----
 fs/fscache/internal.h          |    2 +-
 fs/fscache/netfs.c             |    2 +-
 include/trace/events/fscache.h |   24 +++++++-----------------
 4 files changed, 15 insertions(+), 23 deletions(-)

Comments

Jeff Layton Aug. 24, 2021, 2:24 p.m. UTC | #1
On Mon, 2021-06-21 at 22:47 +0100, David Howells wrote:
> fscache_cookie_put() accesses the cookie it has just put inside the

> tracepoint that monitors the change - but this is something it's not

> allowed to do if we didn't reduce the count to zero.


Do you mean "if the count went to zero." ?

> 

> Fix this by dropping most of those values from the tracepoint and grabbing

> the cookie debug ID before doing the dec.

> 

> Also take the opportunity to switch over the usage and where arguments on

> the tracepoint to put the reason last.

> 

> Signed-off-by: David Howells <dhowells@redhat.com>

> ---

> 

>  fs/fscache/cookie.c            |   10 ++++++----

>  fs/fscache/internal.h          |    2 +-

>  fs/fscache/netfs.c             |    2 +-

>  include/trace/events/fscache.h |   24 +++++++-----------------

>  4 files changed, 15 insertions(+), 23 deletions(-)

> 

> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c

> index 2558814193e9..6df3732cf1b4 100644

> --- a/fs/fscache/cookie.c

> +++ b/fs/fscache/cookie.c

> @@ -225,8 +225,8 @@ struct fscache_cookie *fscache_hash_cookie(struct fscache_cookie *candidate)

>  

>  collision:

>  	if (test_and_set_bit(FSCACHE_COOKIE_ACQUIRED, &cursor->flags)) {

> -		trace_fscache_cookie(cursor, fscache_cookie_collision,

> -				     atomic_read(&cursor->usage));

> +		trace_fscache_cookie(cursor->debug_id, atomic_read(&cursor->usage),

> +				     fscache_cookie_collision);

>  		pr_err("Duplicate cookie detected\n");

>  		fscache_print_cookie(cursor, 'O');

>  		fscache_print_cookie(candidate, 'N');

> @@ -305,7 +305,8 @@ struct fscache_cookie *__fscache_acquire_cookie(

>  

>  	cookie = fscache_hash_cookie(candidate);

>  	if (!cookie) {

> -		trace_fscache_cookie(candidate, fscache_cookie_discard, 1);

> +		trace_fscache_cookie(candidate->debug_id, 1,

> +				     fscache_cookie_discard);

>  		goto out;

>  	}

>  

> @@ -866,8 +867,9 @@ void fscache_cookie_put(struct fscache_cookie *cookie,

>  	_enter("%x", cookie->debug_id);

>  

>  	do {

> +		unsigned int cookie_debug_id = cookie->debug_id;

>  		usage = atomic_dec_return(&cookie->usage);

> -		trace_fscache_cookie(cookie, where, usage);

> +		trace_fscache_cookie(cookie_debug_id, usage, where);

>  

>  		if (usage > 0)

>  			return;

> diff --git a/fs/fscache/internal.h b/fs/fscache/internal.h

> index a49136c63e4b..345105dbbfd1 100644

> --- a/fs/fscache/internal.h

> +++ b/fs/fscache/internal.h

> @@ -291,7 +291,7 @@ static inline void fscache_cookie_get(struct fscache_cookie *cookie,

>  {

>  	int usage = atomic_inc_return(&cookie->usage);

>  

> -	trace_fscache_cookie(cookie, where, usage);

> +	trace_fscache_cookie(cookie->debug_id, usage, where);

>  }

>  

>  /*

> diff --git a/fs/fscache/netfs.c b/fs/fscache/netfs.c

> index cce92216fa28..d6bdb7b5e723 100644

> --- a/fs/fscache/netfs.c

> +++ b/fs/fscache/netfs.c

> @@ -37,7 +37,7 @@ int __fscache_register_netfs(struct fscache_netfs *netfs)

>  	if (!cookie)

>  		goto already_registered;

>  	if (cookie != candidate) {

> -		trace_fscache_cookie(candidate, fscache_cookie_discard, 1);

> +		trace_fscache_cookie(candidate->debug_id, 1, fscache_cookie_discard);

>  		fscache_free_cookie(candidate);

>  	}

>  

> diff --git a/include/trace/events/fscache.h b/include/trace/events/fscache.h

> index 0b9e058aba4d..55b8802740fa 100644

> --- a/include/trace/events/fscache.h

> +++ b/include/trace/events/fscache.h

> @@ -160,37 +160,27 @@ fscache_cookie_traces;

>  

>  

>  TRACE_EVENT(fscache_cookie,

> -	    TP_PROTO(struct fscache_cookie *cookie,

> -		     enum fscache_cookie_trace where,

> -		     int usage),

> +	    TP_PROTO(unsigned int cookie_debug_id,

> +		     int usage,

> +		     enum fscache_cookie_trace where),

>  

> -	    TP_ARGS(cookie, where, usage),

> +	    TP_ARGS(cookie_debug_id, usage, where),

>  

>  	    TP_STRUCT__entry(

>  		    __field(unsigned int,		cookie		)

> -		    __field(unsigned int,		parent		)

>  		    __field(enum fscache_cookie_trace,	where		)

>  		    __field(int,			usage		)

> -		    __field(int,			n_children	)

> -		    __field(int,			n_active	)

> -		    __field(u8,				flags		)

>  			     ),

>  

>  	    TP_fast_assign(

> -		    __entry->cookie	= cookie->debug_id;

> -		    __entry->parent	= cookie->parent ? cookie->parent->debug_id : 0;

> +		    __entry->cookie	= cookie_debug_id;

>  		    __entry->where	= where;

>  		    __entry->usage	= usage;

> -		    __entry->n_children	= atomic_read(&cookie->n_children);

> -		    __entry->n_active	= atomic_read(&cookie->n_active);

> -		    __entry->flags	= cookie->flags;

>  			   ),

>  

> -	    TP_printk("%s c=%08x u=%d p=%08x Nc=%d Na=%d f=%02x",

> +	    TP_printk("%s c=%08x u=%d",

>  		      __print_symbolic(__entry->where, fscache_cookie_traces),

> -		      __entry->cookie, __entry->usage,

> -		      __entry->parent, __entry->n_children, __entry->n_active,

> -		      __entry->flags)

> +		      __entry->cookie, __entry->usage)

>  	    );

>  

>  TRACE_EVENT(fscache_netfs,

> 

> 


Patch itself looks fine though.
-- 
Jeff Layton <jlayton@redhat.com>
David Howells Aug. 25, 2021, 2:05 p.m. UTC | #2
Jeff Layton <jlayton@redhat.com> wrote:

> > fscache_cookie_put() accesses the cookie it has just put inside the

> > tracepoint that monitors the change - but this is something it's not

> > allowed to do if we didn't reduce the count to zero.

> 

> Do you mean "if the count went to zero." ?


No.  If *we* reduced the count to zero, it falls to us to destroy the object,
so we're allowed to look into it again.

If we didn't reduce the count to zero, then someone else might destroy it
before we look into it again.

David
diff mbox series

Patch

diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index 2558814193e9..6df3732cf1b4 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -225,8 +225,8 @@  struct fscache_cookie *fscache_hash_cookie(struct fscache_cookie *candidate)
 
 collision:
 	if (test_and_set_bit(FSCACHE_COOKIE_ACQUIRED, &cursor->flags)) {
-		trace_fscache_cookie(cursor, fscache_cookie_collision,
-				     atomic_read(&cursor->usage));
+		trace_fscache_cookie(cursor->debug_id, atomic_read(&cursor->usage),
+				     fscache_cookie_collision);
 		pr_err("Duplicate cookie detected\n");
 		fscache_print_cookie(cursor, 'O');
 		fscache_print_cookie(candidate, 'N');
@@ -305,7 +305,8 @@  struct fscache_cookie *__fscache_acquire_cookie(
 
 	cookie = fscache_hash_cookie(candidate);
 	if (!cookie) {
-		trace_fscache_cookie(candidate, fscache_cookie_discard, 1);
+		trace_fscache_cookie(candidate->debug_id, 1,
+				     fscache_cookie_discard);
 		goto out;
 	}
 
@@ -866,8 +867,9 @@  void fscache_cookie_put(struct fscache_cookie *cookie,
 	_enter("%x", cookie->debug_id);
 
 	do {
+		unsigned int cookie_debug_id = cookie->debug_id;
 		usage = atomic_dec_return(&cookie->usage);
-		trace_fscache_cookie(cookie, where, usage);
+		trace_fscache_cookie(cookie_debug_id, usage, where);
 
 		if (usage > 0)
 			return;
diff --git a/fs/fscache/internal.h b/fs/fscache/internal.h
index a49136c63e4b..345105dbbfd1 100644
--- a/fs/fscache/internal.h
+++ b/fs/fscache/internal.h
@@ -291,7 +291,7 @@  static inline void fscache_cookie_get(struct fscache_cookie *cookie,
 {
 	int usage = atomic_inc_return(&cookie->usage);
 
-	trace_fscache_cookie(cookie, where, usage);
+	trace_fscache_cookie(cookie->debug_id, usage, where);
 }
 
 /*
diff --git a/fs/fscache/netfs.c b/fs/fscache/netfs.c
index cce92216fa28..d6bdb7b5e723 100644
--- a/fs/fscache/netfs.c
+++ b/fs/fscache/netfs.c
@@ -37,7 +37,7 @@  int __fscache_register_netfs(struct fscache_netfs *netfs)
 	if (!cookie)
 		goto already_registered;
 	if (cookie != candidate) {
-		trace_fscache_cookie(candidate, fscache_cookie_discard, 1);
+		trace_fscache_cookie(candidate->debug_id, 1, fscache_cookie_discard);
 		fscache_free_cookie(candidate);
 	}
 
diff --git a/include/trace/events/fscache.h b/include/trace/events/fscache.h
index 0b9e058aba4d..55b8802740fa 100644
--- a/include/trace/events/fscache.h
+++ b/include/trace/events/fscache.h
@@ -160,37 +160,27 @@  fscache_cookie_traces;
 
 
 TRACE_EVENT(fscache_cookie,
-	    TP_PROTO(struct fscache_cookie *cookie,
-		     enum fscache_cookie_trace where,
-		     int usage),
+	    TP_PROTO(unsigned int cookie_debug_id,
+		     int usage,
+		     enum fscache_cookie_trace where),
 
-	    TP_ARGS(cookie, where, usage),
+	    TP_ARGS(cookie_debug_id, usage, where),
 
 	    TP_STRUCT__entry(
 		    __field(unsigned int,		cookie		)
-		    __field(unsigned int,		parent		)
 		    __field(enum fscache_cookie_trace,	where		)
 		    __field(int,			usage		)
-		    __field(int,			n_children	)
-		    __field(int,			n_active	)
-		    __field(u8,				flags		)
 			     ),
 
 	    TP_fast_assign(
-		    __entry->cookie	= cookie->debug_id;
-		    __entry->parent	= cookie->parent ? cookie->parent->debug_id : 0;
+		    __entry->cookie	= cookie_debug_id;
 		    __entry->where	= where;
 		    __entry->usage	= usage;
-		    __entry->n_children	= atomic_read(&cookie->n_children);
-		    __entry->n_active	= atomic_read(&cookie->n_active);
-		    __entry->flags	= cookie->flags;
 			   ),
 
-	    TP_printk("%s c=%08x u=%d p=%08x Nc=%d Na=%d f=%02x",
+	    TP_printk("%s c=%08x u=%d",
 		      __print_symbolic(__entry->where, fscache_cookie_traces),
-		      __entry->cookie, __entry->usage,
-		      __entry->parent, __entry->n_children, __entry->n_active,
-		      __entry->flags)
+		      __entry->cookie, __entry->usage)
 	    );
 
 TRACE_EVENT(fscache_netfs,