| Message ID | cover.1671898144.git.drv@mailo.com |
|---|---|
| Headers | show |
| Series | tty: serial: dz: convert atomic_* to refcount_* | expand |
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,
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 >
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
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 >
> 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.
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; } }
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
The patch series proposes to transition the driver from using atomic_t APIs to refcount_t APIs for reference count management. Note: patch 2/2 depends on patch 1/2. Hence please apply path 1/2 first. Changes in v3: 1. Patch series introduced rather than individual patches. 2. Update patch subject line to indicate the atomic_t variable being changed Changes in v2: 1. Separate the change to patch per variable rather than combining multiple atomic variable changes into a single patch. Please note: The patches are compile tested using dec_station.defconfig for MIPS architecture. Deepak R Varma (2): tty: serial: dz: convert atomic_* to refcount_* APIs for map_guard tty: serial: dz: convert atomic_* to refcount_* APIs for irq_guard drivers/tty/serial/dz.c | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-) -- 2.34.1