diff mbox series

[v3,1/2] tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard

Message ID d85c7441b96ce387d9010142efc3469d53b6aedc.1671898144.git.drv@mailo.com
State New
Headers
Series tty: serial: dz: convert atomic_* to refcount_* |

Commit Message

Deepak R Varma Dec. 24, 2022, 4:33 p.m. UTC 
  The refcount_* APIs are designed to address known issues with the
atomic_t APIs for reference counting. They provide following distinct
advantages
   - protect the reference counters from overflow/underflow
   - avoid use-after-free errors
   - provide improved memory ordering guarantee schemes
   - neater and safer.
Hence, replace the atomic_* APIs by their equivalent refcount_t
API functions.

This patch proposal address the following warnings generated by
the atomic_as_refcounter.cocci coccinelle script
atomic_add_return(-1, ...)

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please Note:
   1. The patch is compile tested using dec_station.defconfig for MIPS architecture.
   2. This patch should be applied before patch 2/2 of this series due to
      dependency.

Changes in v3:
   1. Include the individual patches in a series and highlight dependency.
      Feedback provided by gregkh@linuxfoundation.org

Changes in v2:
   1. Separate the combined change into one variable per patch as
      suggested by gregkh@linuxfoundation.org
   2. There was additional feedback on validating the change as it appeared to
      modify the existing logic. However, I found that the logic does not
      change with the proposed refcount_* APIs used in this change. Hence that
      feedback is not applied in this version.



 drivers/tty/serial/dz.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

--
2.34.1

Comments

Jiri Slaby Jan. 3, 2023, 8:59 a.m. UTC | #1 
On 26. 12. 22, 7:21, Deepak R Varma wrote:
> The refcount_* APIs are designed to address known issues with the
> atomic_t APIs for reference counting. They provide following distinct
> advantages
>     - protect the reference counters from overflow/underflow
>     - avoid use-after-free errors
>     - provide improved memory ordering guarantee schemes
>     - neater and safer.

Really? (see below)

> --- a/drivers/tty/serial/dz.c
> +++ b/drivers/tty/serial/dz.c
...
> @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
>   static int dz_request_port(struct uart_port *uport)
>   {
>   	struct dz_mux *mux = to_dport(uport)->mux;
> -	int map_guard;
>   	int ret;
> 
> -	map_guard = atomic_add_return(1, &mux->map_guard);
> -	if (map_guard == 1) {
> -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> -					"dz")) {
> -			atomic_add(-1, &mux->map_guard);
> -			printk(KERN_ERR
> -			       "dz: Unable to reserve MMIO resource\n");
> +	refcount_inc(&mux->map_guard);
> +	if (refcount_read(&mux->map_guard) == 1) {

This is now racy, right?

thanks,
Deepak R Varma Jan. 3, 2023, 10:05 a.m. UTC | #2 
On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > The refcount_* APIs are designed to address known issues with the
> > atomic_t APIs for reference counting. They provide following distinct
> > advantages
> >     - protect the reference counters from overflow/underflow
> >     - avoid use-after-free errors
> >     - provide improved memory ordering guarantee schemes
> >     - neater and safer.
>
> Really? (see below)
>
> > --- a/drivers/tty/serial/dz.c
> > +++ b/drivers/tty/serial/dz.c
> ...
> > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> >   static int dz_request_port(struct uart_port *uport)
> >   {
> >   	struct dz_mux *mux = to_dport(uport)->mux;
> > -	int map_guard;
> >   	int ret;
> >
> > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > -	if (map_guard == 1) {
> > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > -					"dz")) {
> > -			atomic_add(-1, &mux->map_guard);
> > -			printk(KERN_ERR
> > -			       "dz: Unable to reserve MMIO resource\n");
> > +	refcount_inc(&mux->map_guard);
> > +	if (refcount_read(&mux->map_guard) == 1) {
>
> This is now racy, right?

Hello Jiri,
Thank you for the feedback. You are correct. I have split a single instruction
in two (or more?) instructions potentially resulting in race conditions. I
looked through the refcount_* APIs but did not find a direct match.


Can you please comment if the the following variation will avoid race condition?

	if (!refcount_add_not_zero(1, &mux->map_guard)) {
		refcount_inc(&mux->map_guard);
		...
	}

Here, refcount_add_not_zero would return false if &mux->map_guard is already 0.
Which means, incrementing it by 1 would have met the earlier if evaluation.
Whereas, if &mux->map_guard is something other than 0, refcount_add_not_zero
will increment it by 1 and return true, in which case the if condition will
fail, similar to the previous if evaluation.

Hope that helps clarify my revised thought. Can you please let me know if this
revision looks safe?

Thank you,
./drv



>
> thanks,
> --
> js
> suse labs
>
Greg KH Jan. 4, 2023, 8:28 a.m. UTC | #3 
On Tue, Jan 03, 2023 at 03:35:15PM +0530, Deepak R Varma wrote:
> On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > The refcount_* APIs are designed to address known issues with the
> > > atomic_t APIs for reference counting. They provide following distinct
> > > advantages
> > >     - protect the reference counters from overflow/underflow
> > >     - avoid use-after-free errors
> > >     - provide improved memory ordering guarantee schemes
> > >     - neater and safer.
> >
> > Really? (see below)
> >
> > > --- a/drivers/tty/serial/dz.c
> > > +++ b/drivers/tty/serial/dz.c
> > ...
> > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > >   static int dz_request_port(struct uart_port *uport)
> > >   {
> > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > -	int map_guard;
> > >   	int ret;
> > >
> > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > -	if (map_guard == 1) {
> > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > -					"dz")) {
> > > -			atomic_add(-1, &mux->map_guard);
> > > -			printk(KERN_ERR
> > > -			       "dz: Unable to reserve MMIO resource\n");
> > > +	refcount_inc(&mux->map_guard);
> > > +	if (refcount_read(&mux->map_guard) == 1) {
> >
> > This is now racy, right?
> 
> Hello Jiri,
> Thank you for the feedback. You are correct. I have split a single instruction
> in two (or more?) instructions potentially resulting in race conditions. I
> looked through the refcount_* APIs but did not find a direct match.
> 
> 
> Can you please comment if the the following variation will avoid race condition?
> 
> 	if (!refcount_add_not_zero(1, &mux->map_guard)) {
> 		refcount_inc(&mux->map_guard);
> 		...
> 	}

What do you think?  The onus is on you to prove the conversion is
correct, otherwise, why do the conversion at all?

Actually, why do this at all, what is the goal here?  And how was this
tested?

thanks,

greg k-h
Deepak R Varma Jan. 4, 2023, 8:59 a.m. UTC | #4 
On Wed, Jan 04, 2023 at 09:28:13AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 03, 2023 at 03:35:15PM +0530, Deepak R Varma wrote:
> > > > -			printk(KERN_ERR
> > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > +	refcount_inc(&mux->map_guard);
> > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > >
> > > This is now racy, right?
> >
> > Hello Jiri,
> > Thank you for the feedback. You are correct. I have split a single instruction
> > in two (or more?) instructions potentially resulting in race conditions. I
> > looked through the refcount_* APIs but did not find a direct match.
> >
> >
> > Can you please comment if the the following variation will avoid race condition?
> >
> > 	if (!refcount_add_not_zero(1, &mux->map_guard)) {
> > 		refcount_inc(&mux->map_guard);
> > 		...
> > 	}
>
> What do you think?  The onus is on you to prove the conversion is
> correct, otherwise, why do the conversion at all?

Hello Greg,
Okay. Sounds good. I think the revised approach should be safer. I will work on
finding a means to prove that.

>
> Actually, why do this at all, what is the goal here?  And how was this
> tested?

The objective here is to migrate to specific and improved APIs that are already
proved to be better for different reasons as mentioned in the patch log
messages. This is as per the Linux Kernel documentation.

In terms of testing, First, I did a compile and build test of the changes.
I also wrote separate small dummy modules and tested the API transformation.
However, these modules were standalone and limited in complexity and intensity.
I will try to make these more intense, multithreaded and run the test again.

Thank you as always :)
./drv

>
> thanks,
>
> greg k-h
Deepak R Varma Jan. 10, 2023, 6:19 a.m. UTC | #5 
On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > The refcount_* APIs are designed to address known issues with the
> > atomic_t APIs for reference counting. They provide following distinct
> > advantages
> >     - protect the reference counters from overflow/underflow
> >     - avoid use-after-free errors
> >     - provide improved memory ordering guarantee schemes
> >     - neater and safer.
> 
> Really? (see below)
> 
> > --- a/drivers/tty/serial/dz.c
> > +++ b/drivers/tty/serial/dz.c
> ...
> > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> >   static int dz_request_port(struct uart_port *uport)
> >   {
> >   	struct dz_mux *mux = to_dport(uport)->mux;
> > -	int map_guard;
> >   	int ret;
> > 
> > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > -	if (map_guard == 1) {
> > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > -					"dz")) {
> > -			atomic_add(-1, &mux->map_guard);
> > -			printk(KERN_ERR
> > -			       "dz: Unable to reserve MMIO resource\n");
> > +	refcount_inc(&mux->map_guard);
> > +	if (refcount_read(&mux->map_guard) == 1) {
> 
> This is now racy, right?

Hello Jiri,
I found this [1] commit which introduced similar transformation in a
neighbouring driver. Can you please comment how is this different from the
current patch proposal?

[1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to refcount_t")

On a side note, I have not been able to find an exact 1:1 map to the
atomic_add_result API. I am wondering should we have one?

Thank you,
./drv


Thank you,
./drv

> 
> thanks,
> -- 
> js
> suse labs
>
Reshetova, Elena Jan. 10, 2023, 7:27 a.m. UTC | #6 
> On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > The refcount_* APIs are designed to address known issues with the
> > > atomic_t APIs for reference counting. They provide following distinct
> > > advantages
> > >     - protect the reference counters from overflow/underflow
> > >     - avoid use-after-free errors
> > >     - provide improved memory ordering guarantee schemes
> > >     - neater and safer.
> >
> > Really? (see below)
> >
> > > --- a/drivers/tty/serial/dz.c
> > > +++ b/drivers/tty/serial/dz.c
> > ...
> > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > >   static int dz_request_port(struct uart_port *uport)
> > >   {
> > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > -	int map_guard;
> > >   	int ret;
> > >
> > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > -	if (map_guard == 1) {
> > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > -					"dz")) {
> > > -			atomic_add(-1, &mux->map_guard);
> > > -			printk(KERN_ERR
> > > -			       "dz: Unable to reserve MMIO resource\n");
> > > +	refcount_inc(&mux->map_guard);
> > > +	if (refcount_read(&mux->map_guard) == 1) {
> >
> > This is now racy, right?
> 
> Hello Jiri,
> I found this [1] commit which introduced similar transformation in a
> neighbouring driver. Can you please comment how is this different from the
> current patch proposal?
> 
> [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> refcount_t")
> 
> On a side note, I have not been able to find an exact 1:1 map to the
> atomic_add_result API. I am wondering should we have one?

In past we have decided not to provide this API for refcount_t
because for truly correctly behaving reference counters it should not be needed
(vs atomics that cover a broader range of use cases). 
Can you use !refcount_inc_not_zero in the above case?

Best Regards,
Elena.
Deepak R Varma Jan. 10, 2023, 7:47 a.m. UTC | #7 
On Tue, Jan 10, 2023 at 07:27:44AM +0000, Reshetova, Elena wrote:
>  
> > On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > > The refcount_* APIs are designed to address known issues with the
> > > > atomic_t APIs for reference counting. They provide following distinct
> > > > advantages
> > > >     - protect the reference counters from overflow/underflow
> > > >     - avoid use-after-free errors
> > > >     - provide improved memory ordering guarantee schemes
> > > >     - neater and safer.
> > >
> > > Really? (see below)
> > >
> > > > --- a/drivers/tty/serial/dz.c
> > > > +++ b/drivers/tty/serial/dz.c
> > > ...
> > > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > > >   static int dz_request_port(struct uart_port *uport)
> > > >   {
> > > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > > -	int map_guard;
> > > >   	int ret;
> > > >
> > > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > > -	if (map_guard == 1) {
> > > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > > -					"dz")) {
> > > > -			atomic_add(-1, &mux->map_guard);
> > > > -			printk(KERN_ERR
> > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > +	refcount_inc(&mux->map_guard);
> > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > >
> > > This is now racy, right?
> > 
> > Hello Jiri,
> > I found this [1] commit which introduced similar transformation in a
> > neighbouring driver. Can you please comment how is this different from the
> > current patch proposal?
> > 
> > [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> > refcount_t")
> > 
> > On a side note, I have not been able to find an exact 1:1 map to the
> > atomic_add_result API. I am wondering should we have one?
> 

Hello Elena,

> In past we have decided not to provide this API for refcount_t
> because for truly correctly behaving reference counters it should not be needed
> (vs atomics that cover a broader range of use cases). 

So, there is no FAA refcount wrapper? I think this is a pretty common need.
Please correct me if I am wrong.

> Can you use !refcount_inc_not_zero in the above case?

I actually did try that but was not sure if truly addresses the objection.
Please attached and let me know if you have a feedback on the alternate
approach.

Thank you,
./drv


> 
> Best Regards,
> Elena.
############## ORIGINAL CODE ##################################
-       map_guard = atomic_add_return(1, &mux->map_guard);
-       if (map_guard == 1) {
-               if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
-                                       "dz")) {
-                       atomic_add(-1, &mux->map_guard);
-                       printk(KERN_ERR
-                              "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }

############## INITIAL APPROACH ##################################
+       refcount_inc(&mux->map_guard);
+       if (refcount_read(&mux->map_guard) == 1) {
+               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+                       refcount_dec(&mux->map_guard);
+                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }

############## ALTERNATE APPROACH ##################################

+       if (!refcount_inc_not_zero(&mux->map_guard)) {
+               refcount_inc(&mux->map_guard);
+               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+                       refcount_dec(&mux->map_guard);
+                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
                        return -EBUSY;
                }
        }
Greg KH Jan. 10, 2023, 7:57 a.m. UTC | #8 
On Tue, Jan 10, 2023 at 01:17:54PM +0530, Deepak R Varma wrote:
> On Tue, Jan 10, 2023 at 07:27:44AM +0000, Reshetova, Elena wrote:
> >  
> > > On Tue, Jan 03, 2023 at 09:59:52AM +0100, Jiri Slaby wrote:
> > > > On 26. 12. 22, 7:21, Deepak R Varma wrote:
> > > > > The refcount_* APIs are designed to address known issues with the
> > > > > atomic_t APIs for reference counting. They provide following distinct
> > > > > advantages
> > > > >     - protect the reference counters from overflow/underflow
> > > > >     - avoid use-after-free errors
> > > > >     - provide improved memory ordering guarantee schemes
> > > > >     - neater and safer.
> > > >
> > > > Really? (see below)
> > > >
> > > > > --- a/drivers/tty/serial/dz.c
> > > > > +++ b/drivers/tty/serial/dz.c
> > > > ...
> > > > > @@ -687,23 +686,19 @@ static int dz_map_port(struct uart_port *uport)
> > > > >   static int dz_request_port(struct uart_port *uport)
> > > > >   {
> > > > >   	struct dz_mux *mux = to_dport(uport)->mux;
> > > > > -	int map_guard;
> > > > >   	int ret;
> > > > >
> > > > > -	map_guard = atomic_add_return(1, &mux->map_guard);
> > > > > -	if (map_guard == 1) {
> > > > > -		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> > > > > -					"dz")) {
> > > > > -			atomic_add(-1, &mux->map_guard);
> > > > > -			printk(KERN_ERR
> > > > > -			       "dz: Unable to reserve MMIO resource\n");
> > > > > +	refcount_inc(&mux->map_guard);
> > > > > +	if (refcount_read(&mux->map_guard) == 1) {
> > > >
> > > > This is now racy, right?
> > > 
> > > Hello Jiri,
> > > I found this [1] commit which introduced similar transformation in a
> > > neighbouring driver. Can you please comment how is this different from the
> > > current patch proposal?
> > > 
> > > [1] commit ID: 22a33651a56f ("convert sbd_duart.map_guard from atomic_t to
> > > refcount_t")
> > > 
> > > On a side note, I have not been able to find an exact 1:1 map to the
> > > atomic_add_result API. I am wondering should we have one?
> > 
> 
> Hello Elena,
> 
> > In past we have decided not to provide this API for refcount_t
> > because for truly correctly behaving reference counters it should not be needed
> > (vs atomics that cover a broader range of use cases). 
> 
> So, there is no FAA refcount wrapper? I think this is a pretty common need.
> Please correct me if I am wrong.
> 
> > Can you use !refcount_inc_not_zero in the above case?
> 
> I actually did try that but was not sure if truly addresses the objection.
> Please attached and let me know if you have a feedback on the alternate
> approach.
> 
> Thank you,
> ./drv
> 
> 
> > 
> > Best Regards,
> > Elena.

> ############## ORIGINAL CODE ##################################
> -       map_guard = atomic_add_return(1, &mux->map_guard);
> -       if (map_guard == 1) {
> -               if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
> -                                       "dz")) {
> -                       atomic_add(-1, &mux->map_guard);
> -                       printk(KERN_ERR
> -                              "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 
> ############## INITIAL APPROACH ##################################
> +       refcount_inc(&mux->map_guard);
> +       if (refcount_read(&mux->map_guard) == 1) {
> +               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
> +                       refcount_dec(&mux->map_guard);
> +                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 
> ############## ALTERNATE APPROACH ##################################
> 
> +       if (!refcount_inc_not_zero(&mux->map_guard)) {
> +               refcount_inc(&mux->map_guard);
> +               if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
> +                       refcount_dec(&mux->map_guard);
> +                       printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
>                         return -EBUSY;
>                 }
>         }
> 

This feels odd to me, why not just use a normal lock instead?

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index 6b7ed7f2f3ca..b70edc248f8b 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -47,6 +47,7 @@ 
 #include <linux/tty_flip.h>

 #include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/io.h>
 #include <asm/bootinfo.h>

@@ -75,7 +76,7 @@  struct dz_port {

 struct dz_mux {
 	struct dz_port		dport[DZ_NB_PORT];
-	atomic_t		map_guard;
+	refcount_t		map_guard;
 	atomic_t		irq_guard;
 	int			initialised;
 };
@@ -662,13 +663,11 @@  static const char *dz_type(struct uart_port *uport)
 static void dz_release_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;

 	iounmap(uport->membase);
 	uport->membase = NULL;

-	map_guard = atomic_add_return(-1, &mux->map_guard);
-	if (!map_guard)
+	if (refcount_dec_and_test(&mux->map_guard))
 		release_mem_region(uport->mapbase, dec_kn_slot_size);
 }

@@ -687,23 +686,19 @@  static int dz_map_port(struct uart_port *uport)
 static int dz_request_port(struct uart_port *uport)
 {
 	struct dz_mux *mux = to_dport(uport)->mux;
-	int map_guard;
 	int ret;

-	map_guard = atomic_add_return(1, &mux->map_guard);
-	if (map_guard == 1) {
-		if (!request_mem_region(uport->mapbase, dec_kn_slot_size,
-					"dz")) {
-			atomic_add(-1, &mux->map_guard);
-			printk(KERN_ERR
-			       "dz: Unable to reserve MMIO resource\n");
+	refcount_inc(&mux->map_guard);
+	if (refcount_read(&mux->map_guard) == 1) {
+		if (!request_mem_region(uport->mapbase, dec_kn_slot_size, "dz")) {
+			refcount_dec(&mux->map_guard);
+			printk(KERN_ERR "dz: Unable to reserve MMIO resource\n");
 			return -EBUSY;
 		}
 	}
 	ret = dz_map_port(uport);
 	if (ret) {
-		map_guard = atomic_add_return(-1, &mux->map_guard);
-		if (!map_guard)
+		if (refcount_dec_and_test(&mux->map_guard))
 			release_mem_region(uport->mapbase, dec_kn_slot_size);
 		return ret;
 	}