Re: [PATCH 1/2] liveupdate: Use refcount_t for FLB reference counts

[Pasha Tatashin]

On 04-23 17:40, David Matlack wrote:
> Use refcount_t instead of a raw integer to keep track of references on
> incoming and outgoing FLBs. Using refcount_t provides protection from
> overflow, underflow, and other issues.
> 
> Fixes: cab056f2aae7 ("liveupdate: luo_flb: introduce File-Lifecycle-Bound global state")
> Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> ---
>  include/linux/liveupdate.h  |  3 ++-
>  kernel/liveupdate/luo_flb.c | 22 ++++++++++------------
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/liveupdate.h b/include/linux/liveupdate.h
> index 30c5a39ff9e9..8d3bbc35c828 100644
> --- a/include/linux/liveupdate.h
> +++ b/include/linux/liveupdate.h
> @@ -12,6 +12,7 @@
>  #include <linux/kho/abi/luo.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/refcount.h>
>  #include <linux/rwsem.h>
>  #include <linux/types.h>
>  #include <uapi/linux/liveupdate.h>
> @@ -175,7 +176,7 @@ struct liveupdate_flb_ops {
>   * @retrieved: True once the FLB's retrieve() callback has run.
>   */
>  struct luo_flb_private_state {
> -	long count;
> +	refcount_t count;
>  	u64 data;
>  	void *obj;
>  	struct mutex lock;
> diff --git a/kernel/liveupdate/luo_flb.c b/kernel/liveupdate/luo_flb.c
> index 00f5494812c4..59c5f31ab767 100644
> --- a/kernel/liveupdate/luo_flb.c
> +++ b/kernel/liveupdate/luo_flb.c
> @@ -111,7 +111,7 @@ static int luo_flb_file_preserve_one(struct liveupdate_flb *flb)
>  	struct luo_flb_private *private = luo_flb_get_private(flb);
>  
>  	scoped_guard(mutex, &private->outgoing.lock) {
> -		if (!private->outgoing.count) {
> +		if (!refcount_read(&private->outgoing.count)) {
>  			struct liveupdate_flb_op_args args = {0};
>  			int err;
>  
> @@ -126,8 +126,10 @@ static int luo_flb_file_preserve_one(struct liveupdate_flb *flb)
>  			}
>  			private->outgoing.data = args.data;
>  			private->outgoing.obj = args.obj;
> +			refcount_set(&private->outgoing.count, 1);
> +		} else {
> +			refcount_inc(&private->outgoing.count);
>  		}
> -		private->outgoing.count++;

It should be: refcount_inc(&private->outgoing.count); for both
cases, as it was before.

Additionally, please add refcount_set(&private->outgoing.count, 0) to
luo_flb_get_private, where the rest of the private fields are 
initialized.

In general, I prefer to avoid refcount_set() because it breaks 
continuity and makes debugging hard. It should only be used only during 
initialization, so moving it to where the other fields are initialized 
is the correct approach.

>  	}
>  
>  	return 0;
> @@ -138,8 +140,7 @@ static void luo_flb_file_unpreserve_one(struct liveupdate_flb *flb)
>  	struct luo_flb_private *private = luo_flb_get_private(flb);
>  
>  	scoped_guard(mutex, &private->outgoing.lock) {
> -		private->outgoing.count--;
> -		if (!private->outgoing.count) {
> +		if (refcount_dec_and_test(&private->outgoing.count)) {
>  			struct liveupdate_flb_op_args args = {0};
>  
>  			args.flb = flb;
> @@ -178,7 +179,7 @@ static int luo_flb_retrieve_one(struct liveupdate_flb *flb)
>  	for (int i = 0; i < fh->header_ser->count; i++) {
>  		if (!strcmp(fh->ser[i].name, flb->compatible)) {
>  			private->incoming.data = fh->ser[i].data;
> -			private->incoming.count = fh->ser[i].count;
> +			refcount_set(&private->incoming.count, fh->ser[i].count);
>  			found = true;
>  			break;
>  		}
> @@ -208,12 +209,8 @@ static int luo_flb_retrieve_one(struct liveupdate_flb *flb)
>  static void luo_flb_file_finish_one(struct liveupdate_flb *flb)
>  {
>  	struct luo_flb_private *private = luo_flb_get_private(flb);
> -	u64 count;
>  
> -	scoped_guard(mutex, &private->incoming.lock)
> -		count = --private->incoming.count;
> -
> -	if (!count) {
> +	if (refcount_dec_and_test(&private->incoming.count)) {
>  		struct liveupdate_flb_op_args args = {0};
>  
>  		if (!private->incoming.retrieved) {
> @@ -652,12 +649,13 @@ void luo_flb_serialize(void)
>  	guard(rwsem_read)(&luo_register_rwlock);
>  	list_private_for_each_entry(gflb, &luo_flb_global.list, private.list) {
>  		struct luo_flb_private *private = luo_flb_get_private(gflb);
> +		long count = refcount_read(&private->outgoing.count);
>  
> -		if (private->outgoing.count > 0) {
> +		if (count > 0) {
>  			strscpy(fh->ser[i].name, gflb->compatible,
>  				sizeof(fh->ser[i].name));
>  			fh->ser[i].data = private->outgoing.data;
> -			fh->ser[i].count = private->outgoing.count;
> +			fh->ser[i].count = count;
>  			i++;
>  		}
>  	}
> -- 
> 2.54.0.rc2.544.gc7ae2d5bb8-goog
>