Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 Jan 2023 15:35:15 +0530
From:   Deepak R Varma <drv@mailo.com>
To:     Jiri Slaby <jirislaby@kernel.org>
Cc:     "Maciej W. Rozycki" <macro@orcam.me.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
        Saurabh Singh Sengar <ssengar@microsoft.com>,
        Praveen Kumar <kumarpraveen@linux.microsoft.com>,
        Deepak R Varma <drv@mailo.com>
Subject: Re: [PATCH v4 1/2] tty: serial: dz: convert atomic_* to refcount_*
 APIs for map_guard
Message-ID: <Y7P926/+N9IDKCyR@qemulion>
References: <cover.1671898144.git.drv@mailo.com>
 <d85c7441b96ce387d9010142efc3469d53b6aedc.1671898144.git.drv@mailo.com>
 <e42d5d19-7ed5-468b-98cc-13d0187dc555@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e42d5d19-7ed5-468b-98cc-13d0187dc555@kernel.org>
Precedence: bulk

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
>