usb: gadget: u_serial: Add null pointer check in gserial_resume

  Consider a case where gserial_disconnect has already cleared
gser->ioport. And if a wakeup interrupt triggers afterwards,
gserial_resume gets called, which will lead to accessing of
gserial->port and thus causing null pointer dereference.Add
a null pointer check to prevent this.

Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
 drivers/usb/gadget/function/u_serial.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
> Consider a case where gserial_disconnect has already cleared
> gser->ioport. And if a wakeup interrupt triggers afterwards,
> gserial_resume gets called, which will lead to accessing of
> gserial->port and thus causing null pointer dereference.Add
> a null pointer check to prevent this.
> 
> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")

Nit, and our tools will complain, no " " before the "usb:" string here,
right?



> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> ---
>  drivers/usb/gadget/function/u_serial.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 840626e..98be2b8 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
>  	struct gs_port *port = gser->ioport;
>  	unsigned long	flags;
>  
> +	if (!port)
> +		return;
> +

What prevents port from going to NULL right after this check?

thanks,

greg k-h
  

On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
>> Consider a case where gserial_disconnect has already cleared
>> gser->ioport. And if a wakeup interrupt triggers afterwards,
>> gserial_resume gets called, which will lead to accessing of
>> gserial->port and thus causing null pointer dereference.Add
>> a null pointer check to prevent this.
>>
>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
> 
> Nit, and our tools will complain, no " " before the "usb:" string here,
> right?
> 
Will fix it in next patch.
> 
> 
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>> ---
>>   drivers/usb/gadget/function/u_serial.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>> index 840626e..98be2b8 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
>>   	struct gs_port *port = gser->ioport;
>>   	unsigned long	flags;
>>   
>> +	if (!port)
>> +		return;
>> +
> 
> What prevents port from going to NULL right after this check?
In our case we got a null pointer de-reference while performing USB 
compliance tests, as the gser->port was null. Because in gserial_resume, 
spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was 
already marked null by gserial_disconnect.

And after gserial_resume acquires the spinlock, gserial_disconnect cant 
mark it null until the spinlock is released. We need to check if the 
port->lock is valid before accessing it, otherwise it can lead to the 
above mentioned scenario

Issue Type: kernel panic issue
Issue AutoSignature:
pc : do_raw_spin_lock
lr : _raw_spin_lock_irqsave
Call trace:
do_raw_spin_lock
_raw_spin_lock_irqsave
gserial_resume
acm_resume
composite_resume
configfs_composite_resume
dwc3_process_event_entry
dwc3_process_event_buf
dwc3_thread_interrupt
irq_thread_fn
irq_thread
kthread
ret_from_fork

Thanks in advance,
Prashanth
> 
> thanks,
> 
> greg k-h
  

On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote:
> 
> 
> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
> > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
> > > Consider a case where gserial_disconnect has already cleared
> > > gser->ioport. And if a wakeup interrupt triggers afterwards,
> > > gserial_resume gets called, which will lead to accessing of
> > > gserial->port and thus causing null pointer dereference.Add
> > > a null pointer check to prevent this.
> > > 
> > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
> > 
> > Nit, and our tools will complain, no " " before the "usb:" string here,
> > right?
> > 
> Will fix it in next patch.
> > 
> > 
> > > Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> > > ---
> > >   drivers/usb/gadget/function/u_serial.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> > > index 840626e..98be2b8 100644
> > > --- a/drivers/usb/gadget/function/u_serial.c
> > > +++ b/drivers/usb/gadget/function/u_serial.c
> > > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
> > >   	struct gs_port *port = gser->ioport;
> > >   	unsigned long	flags;
> > > +	if (!port)
> > > +		return;
> > > +
> > 
> > What prevents port from going to NULL right after this check?
> In our case we got a null pointer de-reference while performing USB
> compliance tests, as the gser->port was null. Because in gserial_resume,
> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was
> already marked null by gserial_disconnect.
> 
> And after gserial_resume acquires the spinlock, gserial_disconnect cant mark
> it null until the spinlock is released. We need to check if the port->lock
> is valid before accessing it, otherwise it can lead to the above mentioned
> scenario

What happens if gserial_disconnect sets gser->port to NULL immediately 
after your new check occurs, but before 
spinlock_irq_save(&port->port_lock) gets called?

You may need to add a static spinlock to prevent this from happening.

Alan Stern
  

On 09-02-23 01:51 am, Alan Stern wrote:
> On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote:
>>
>>
>> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
>>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
>>>> Consider a case where gserial_disconnect has already cleared
>>>> gser->ioport. And if a wakeup interrupt triggers afterwards,
>>>> gserial_resume gets called, which will lead to accessing of
>>>> gserial->port and thus causing null pointer dereference.Add
>>>> a null pointer check to prevent this.
>>>>
>>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
>>>
>>> Nit, and our tools will complain, no " " before the "usb:" string here,
>>> right?
>>>
>> Will fix it in next patch.
>>>
>>>
>>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>>>> ---
>>>>    drivers/usb/gadget/function/u_serial.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>>>> index 840626e..98be2b8 100644
>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
>>>>    	struct gs_port *port = gser->ioport;
>>>>    	unsigned long	flags;
>>>> +	if (!port)
>>>> +		return;
>>>> +
>>>
>>> What prevents port from going to NULL right after this check?
>> In our case we got a null pointer de-reference while performing USB
>> compliance tests, as the gser->port was null. Because in gserial_resume,
>> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was
>> already marked null by gserial_disconnect.
>>
>> And after gserial_resume acquires the spinlock, gserial_disconnect cant mark
>> it null until the spinlock is released. We need to check if the port->lock
>> is valid before accessing it, otherwise it can lead to the above mentioned
>> scenario
> 
> What happens if gserial_disconnect sets gser->port to NULL immediately
> after your new check occurs, but before
> spinlock_irq_save(&port->port_lock) gets called?
> 
> You may need to add a static spinlock to prevent this from happening.
> 
> Alan Stern
In that case i guess we have to make port_lock a global variable and 
take it out of gs_port structure.

+ static DEFINE_SPINLOCK(port_lock);

struct gs_port {
	struct tty_port port;
-	spinlock_t port_lock;

This will require us to change all the spinlock(port->port_lock) used in 
u_serial.c, what do you suggest?
  

On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote:
> 
> 
> On 09-02-23 01:51 am, Alan Stern wrote:
> > On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote:
> > > 
> > > 
> > > On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
> > > > On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
> > > > > Consider a case where gserial_disconnect has already cleared
> > > > > gser->ioport. And if a wakeup interrupt triggers afterwards,
> > > > > gserial_resume gets called, which will lead to accessing of
> > > > > gserial->port and thus causing null pointer dereference.Add
> > > > > a null pointer check to prevent this.
> > > > > 
> > > > > Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
> > > > 
> > > > Nit, and our tools will complain, no " " before the "usb:" string here,
> > > > right?
> > > > 
> > > Will fix it in next patch.
> > > > 
> > > > 
> > > > > Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> > > > > ---
> > > > >    drivers/usb/gadget/function/u_serial.c | 3 +++
> > > > >    1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> > > > > index 840626e..98be2b8 100644
> > > > > --- a/drivers/usb/gadget/function/u_serial.c
> > > > > +++ b/drivers/usb/gadget/function/u_serial.c
> > > > > @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
> > > > >    	struct gs_port *port = gser->ioport;
> > > > >    	unsigned long	flags;
> > > > > +	if (!port)
> > > > > +		return;
> > > > > +
> > > > 
> > > > What prevents port from going to NULL right after this check?
> > > In our case we got a null pointer de-reference while performing USB
> > > compliance tests, as the gser->port was null. Because in gserial_resume,
> > > spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was
> > > already marked null by gserial_disconnect.
> > > 
> > > And after gserial_resume acquires the spinlock, gserial_disconnect cant mark
> > > it null until the spinlock is released. We need to check if the port->lock
> > > is valid before accessing it, otherwise it can lead to the above mentioned
> > > scenario
> > 
> > What happens if gserial_disconnect sets gser->port to NULL immediately
> > after your new check occurs, but before
> > spinlock_irq_save(&port->port_lock) gets called?
> > 
> > You may need to add a static spinlock to prevent this from happening.
> > 
> > Alan Stern
> In that case i guess we have to make port_lock a global variable and take it
> out of gs_port structure.
> 
> + static DEFINE_SPINLOCK(port_lock);
> 
> struct gs_port {
> 	struct tty_port port;
> -	spinlock_t port_lock;
> 
> This will require us to change all the spinlock(port->port_lock) used in
> u_serial.c, what do you suggest?

Yes, that would be the correct thing to do.
  

On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote:
> On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote:
>>
>>
>> On 09-02-23 01:51 am, Alan Stern wrote:
>>> On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote:
>>>>
>>>>
>>>> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
>>>>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
>>>>>> Consider a case where gserial_disconnect has already cleared
>>>>>> gser->ioport. And if a wakeup interrupt triggers afterwards,
>>>>>> gserial_resume gets called, which will lead to accessing of
>>>>>> gserial->port and thus causing null pointer dereference.Add
>>>>>> a null pointer check to prevent this.
>>>>>>
>>>>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume callbacks")
>>>>>
>>>>> Nit, and our tools will complain, no " " before the "usb:" string here,
>>>>> right?
>>>>>
>>>> Will fix it in next patch.
>>>>>
>>>>>
>>>>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/gadget/function/u_serial.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
>>>>>> index 840626e..98be2b8 100644
>>>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
>>>>>>     	struct gs_port *port = gser->ioport;
>>>>>>     	unsigned long	flags;
>>>>>> +	if (!port)
>>>>>> +		return;
>>>>>> +
>>>>>
>>>>> What prevents port from going to NULL right after this check?
>>>> In our case we got a null pointer de-reference while performing USB
>>>> compliance tests, as the gser->port was null. Because in gserial_resume,
>>>> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port was
>>>> already marked null by gserial_disconnect.
>>>>
>>>> And after gserial_resume acquires the spinlock, gserial_disconnect cant mark
>>>> it null until the spinlock is released. We need to check if the port->lock
>>>> is valid before accessing it, otherwise it can lead to the above mentioned
>>>> scenario
>>>
>>> What happens if gserial_disconnect sets gser->port to NULL immediately
>>> after your new check occurs, but before
>>> spinlock_irq_save(&port->port_lock) gets called?
>>>
>>> You may need to add a static spinlock to prevent this from happening.
>>>
>>> Alan Stern
>> In that case i guess we have to make port_lock a global variable and take it
>> out of gs_port structure.
>>
>> + static DEFINE_SPINLOCK(port_lock);
>>
>> struct gs_port {
>> 	struct tty_port port;
>> -	spinlock_t port_lock;
>>
>> This will require us to change all the spinlock(port->port_lock) used in
>> u_serial.c, what do you suggest?
> 
> Yes, that would be the correct thing to do.
will do it and share next patch

Thanks for the suggestions
Prashanth K
  

On 09-02-23 12:33 pm, Prashanth K wrote:
> 
> 
> On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote:
>> On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote:
>>>
>>>
>>> On 09-02-23 01:51 am, Alan Stern wrote:
>>>> On Wed, Feb 08, 2023 at 09:15:54PM +0530, Prashanth K wrote:
>>>>>
>>>>>
>>>>> On 08-02-23 08:24 pm, Greg Kroah-Hartman wrote:
>>>>>> On Wed, Feb 08, 2023 at 07:24:47PM +0530, Prashanth K wrote:
>>>>>>> Consider a case where gserial_disconnect has already cleared
>>>>>>> gser->ioport. And if a wakeup interrupt triggers afterwards,
>>>>>>> gserial_resume gets called, which will lead to accessing of
>>>>>>> gserial->port and thus causing null pointer dereference.Add
>>>>>>> a null pointer check to prevent this.
>>>>>>>
>>>>>>> Fixes: aba3a8d01d62 (" usb: gadget: u_serial: add suspend resume 
>>>>>>> callbacks")
>>>>>>
>>>>>> Nit, and our tools will complain, no " " before the "usb:" string 
>>>>>> here,
>>>>>> right?
>>>>>>
>>>>> Will fix it in next patch.
>>>>>>
>>>>>>
>>>>>>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
>>>>>>> ---
>>>>>>>     drivers/usb/gadget/function/u_serial.c | 3 +++
>>>>>>>     1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c 
>>>>>>> b/drivers/usb/gadget/function/u_serial.c
>>>>>>> index 840626e..98be2b8 100644
>>>>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>>>>> @@ -1428,6 +1428,9 @@ void gserial_resume(struct gserial *gser)
>>>>>>>         struct gs_port *port = gser->ioport;
>>>>>>>         unsigned long    flags;
>>>>>>> +    if (!port)
>>>>>>> +        return;
>>>>>>> +
>>>>>>
>>>>>> What prevents port from going to NULL right after this check?
>>>>> In our case we got a null pointer de-reference while performing USB
>>>>> compliance tests, as the gser->port was null. Because in 
>>>>> gserial_resume,
>>>>> spinlock_irq_save(&port->port_lock) accesses a null-pointer as port 
>>>>> was
>>>>> already marked null by gserial_disconnect.
>>>>>
>>>>> And after gserial_resume acquires the spinlock, gserial_disconnect 
>>>>> cant mark
>>>>> it null until the spinlock is released. We need to check if the 
>>>>> port->lock
>>>>> is valid before accessing it, otherwise it can lead to the above 
>>>>> mentioned
>>>>> scenario
>>>>
>>>> What happens if gserial_disconnect sets gser->port to NULL immediately
>>>> after your new check occurs, but before
>>>> spinlock_irq_save(&port->port_lock) gets called?
>>>>
>>>> You may need to add a static spinlock to prevent this from happening.
>>>>
>>>> Alan Stern
>>> In that case i guess we have to make port_lock a global variable and 
>>> take it
>>> out of gs_port structure.
>>>
>>> + static DEFINE_SPINLOCK(port_lock);
>>>
>>> struct gs_port {
>>>     struct tty_port port;
>>> -    spinlock_t port_lock;
>>>
>>> This will require us to change all the spinlock(port->port_lock) used in
>>> u_serial.c, what do you suggest?
>>
>> Yes, that would be the correct thing to do.
Hi Greg/Alan, One general doubt, if we make the spinlock static/global, 
wouldn't that be a problem when there are multiple instances, and also 
multiple interfaces can use u_serial at same time. Asking this because 
u_serial can be used by f_serial (gser) as well as f_acm (acm).

Thanks
Prahanth K

> will do it and share next patch
> 
> Thanks for the suggestions
> Prashanth K
  

On Thu, Feb 09, 2023 at 07:37:01PM +0530, Prashanth K wrote:
> 
> 
> On 09-02-23 12:33 pm, Prashanth K wrote:
> > 
> > 
> > On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote:
> > > > In that case i guess we have to make port_lock a global variable
> > > > and take it
> > > > out of gs_port structure.
> > > > 
> > > > + static DEFINE_SPINLOCK(port_lock);
> > > > 
> > > > struct gs_port {
> > > >     struct tty_port port;
> > > > -    spinlock_t port_lock;
> > > > 
> > > > This will require us to change all the spinlock(port->port_lock) used in
> > > > u_serial.c, what do you suggest?
> > > 
> > > Yes, that would be the correct thing to do.
> Hi Greg/Alan, One general doubt, if we make the spinlock static/global,
> wouldn't that be a problem when there are multiple instances, and also
> multiple interfaces can use u_serial at same time. Asking this because
> u_serial can be used by f_serial (gser) as well as f_acm (acm).

You should consider having _two_ spinlocks: One in the gs_port structure 
(the way it is now) and a separate global lock.  The first would be used 
in situations where you know you have a valid pointer.  The second would 
be used in situations where you don't know if the pointer is non-NULL 
or where you are changing the pointer's value.

Alan Stern
  

On 09-02-23 08:39 pm, Alan Stern wrote:
> On Thu, Feb 09, 2023 at 07:37:01PM +0530, Prashanth K wrote:
>>
>>
>> On 09-02-23 12:33 pm, Prashanth K wrote:
>>>
>>>
>>> On 09-02-23 12:31 pm, Greg Kroah-Hartman wrote:
>>>> On Thu, Feb 09, 2023 at 10:31:50AM +0530, Prashanth K wrote:
>>>>> In that case i guess we have to make port_lock a global variable
>>>>> and take it
>>>>> out of gs_port structure.
>>>>>
>>>>> + static DEFINE_SPINLOCK(port_lock);
>>>>>
>>>>> struct gs_port {
>>>>>      struct tty_port port;
>>>>> -    spinlock_t port_lock;
>>>>>
>>>>> This will require us to change all the spinlock(port->port_lock) used in
>>>>> u_serial.c, what do you suggest?
>>>>
>>>> Yes, that would be the correct thing to do.
>> Hi Greg/Alan, One general doubt, if we make the spinlock static/global,
>> wouldn't that be a problem when there are multiple instances, and also
>> multiple interfaces can use u_serial at same time. Asking this because
>> u_serial can be used by f_serial (gser) as well as f_acm (acm).
> 
> You should consider having _two_ spinlocks: One in the gs_port structure
> (the way it is now) and a separate global lock.  The first would be used
> in situations where you know you have a valid pointer.  The second would
> be used in situations where you don't know if the pointer is non-NULL
> or where you are changing the pointer's value.
Lets say we replaced the existing spinlock in gserial_resume and 
gserial_disconnect with a new static spinlock, and kept the spinlocks in 
other functions unchanged. In that case, wouldn't it cause additional 
race conditions as we are using 2 different locks.

Thanks,
Prashanth K
> 
> Alan Stern
  

On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote:
> 
> 
> On 09-02-23 08:39 pm, Alan Stern wrote:
> > You should consider having _two_ spinlocks: One in the gs_port structure
> > (the way it is now) and a separate global lock.  The first would be used
> > in situations where you know you have a valid pointer.  The second would
> > be used in situations where you don't know if the pointer is non-NULL
> > or where you are changing the pointer's value.
> Lets say we replaced the existing spinlock in gserial_resume and
> gserial_disconnect with a new static spinlock, and kept the spinlocks in
> other functions unchanged. In that case, wouldn't it cause additional race
> conditions as we are using 2 different locks.

Not race conditions, but possibilities for deadlock.

Indeed, you would have to be very careful about avoiding deadlock 
scenarios.  In particular, you would have to ensure that the code never 
tries to acquire the global spinlock while already holding one of the 
per-port spinlocks.

Alan Stern
  

On 09-02-23 09:33 pm, Alan Stern wrote:
> On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote:
>>
>>
>> On 09-02-23 08:39 pm, Alan Stern wrote:
>>> You should consider having _two_ spinlocks: One in the gs_port structure
>>> (the way it is now) and a separate global lock.  The first would be used
>>> in situations where you know you have a valid pointer.  The second would
>>> be used in situations where you don't know if the pointer is non-NULL
>>> or where you are changing the pointer's value.
>> Lets say we replaced the existing spinlock in gserial_resume and
>> gserial_disconnect with a new static spinlock, and kept the spinlocks in
>> other functions unchanged. In that case, wouldn't it cause additional race
>> conditions as we are using 2 different locks.
> 
> Not race conditions, but possibilities for deadlock.
> 
> Indeed, you would have to be very careful about avoiding deadlock
> scenarios.  In particular, you would have to ensure that the code never
> tries to acquire the global spinlock while already holding one of the
> per-port spinlocks.
> 
> Alan Stern
Hi Alan, instead of doing these and causing potential regressions, can 
we just have the null pointer check which i suggested in the beginning? 
The major concern was that port might become null after the null pointer 
check. We mark gser->ioport as null pointer in gserial_disconnect, and 
in gserial_resume we copy the gser->ioport to *port in the beginning.

struct gs_port *port = gser->ioport;

And hence it wont cause null pointer deref after the check as we don't 
de-reference anything from gser->ioport afterwards. We only use the 
local pointer *port afterwards.

Thanks,
Prashanth K
  

On Thu, Feb 09, 2023 at 11:57:17PM +0530, Prashanth K wrote:
> 
> 
> On 09-02-23 09:33 pm, Alan Stern wrote:
> > On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote:
> > > 
> > > 
> > > On 09-02-23 08:39 pm, Alan Stern wrote:
> > > > You should consider having _two_ spinlocks: One in the gs_port structure
> > > > (the way it is now) and a separate global lock.  The first would be used
> > > > in situations where you know you have a valid pointer.  The second would
> > > > be used in situations where you don't know if the pointer is non-NULL
> > > > or where you are changing the pointer's value.
> > > Lets say we replaced the existing spinlock in gserial_resume and
> > > gserial_disconnect with a new static spinlock, and kept the spinlocks in
> > > other functions unchanged. In that case, wouldn't it cause additional race
> > > conditions as we are using 2 different locks.
> > 
> > Not race conditions, but possibilities for deadlock.
> > 
> > Indeed, you would have to be very careful about avoiding deadlock
> > scenarios.  In particular, you would have to ensure that the code never
> > tries to acquire the global spinlock while already holding one of the
> > per-port spinlocks.
> > 
> > Alan Stern
> Hi Alan, instead of doing these and causing potential regressions, can we
> just have the null pointer check which i suggested in the beginning? The
> major concern was that port might become null after the null pointer check.

What you are describing is a data race: gserial_disconnect() can write 
to gser->ioport at the same time that gserial_resume() reads from it.  
Unless you're working on a fast path -- which this isn't -- you should 
strive to avoid data races by using proper locking.  That means adding 
the extra spinlock, or finding some other way to make these two accesses 
be mutually exclusive.

With a little care you can ensure there won't be any regressions.  Just 
do what I said above: Make sure the code never tries to acquire the 
global spinlock while already holding one of the per-port spinlocks.

> We mark gser->ioport as null pointer in gserial_disconnect, and in
> gserial_resume we copy the gser->ioport to *port in the beginning.
> 
> struct gs_port *port = gser->ioport;
> 
> And hence it wont cause null pointer deref after the check as we don't
> de-reference anything from gser->ioport afterwards. We only use the local
> pointer *port afterwards.

You cannot depend on this to work the way you want.  The compiler will 
optimize your source code, and one of the optimizations might be to 
eliminate the "port" variable entirely and replace it with gser->ioport.

Alan Stern
  

On 10-02-23 02:35 am, Alan Stern wrote:
> On Thu, Feb 09, 2023 at 11:57:17PM +0530, Prashanth K wrote:
>>
>>
>> On 09-02-23 09:33 pm, Alan Stern wrote:
>>> On Thu, Feb 09, 2023 at 09:13:37PM +0530, Prashanth K wrote:
>>>>
>>>>
>>>> On 09-02-23 08:39 pm, Alan Stern wrote:
>>>>> You should consider having _two_ spinlocks: One in the gs_port structure
>>>>> (the way it is now) and a separate global lock.  The first would be used
>>>>> in situations where you know you have a valid pointer.  The second would
>>>>> be used in situations where you don't know if the pointer is non-NULL
>>>>> or where you are changing the pointer's value.
>>>> Lets say we replaced the existing spinlock in gserial_resume and
>>>> gserial_disconnect with a new static spinlock, and kept the spinlocks in
>>>> other functions unchanged. In that case, wouldn't it cause additional race
>>>> conditions as we are using 2 different locks.
>>>
>>> Not race conditions, but possibilities for deadlock.
>>>
>>> Indeed, you would have to be very careful about avoiding deadlock
>>> scenarios.  In particular, you would have to ensure that the code never
>>> tries to acquire the global spinlock while already holding one of the
>>> per-port spinlocks.
>>>
>>> Alan Stern
>> Hi Alan, instead of doing these and causing potential regressions, can we
>> just have the null pointer check which i suggested in the beginning? The
>> major concern was that port might become null after the null pointer check.
> 
> What you are describing is a data race: gserial_disconnect() can write
> to gser->ioport at the same time that gserial_resume() reads from it.
> Unless you're working on a fast path -- which this isn't -- you should
> strive to avoid data races by using proper locking.  That means adding
> the extra spinlock, or finding some other way to make these two accesses
> be mutually exclusive.
> 
> With a little care you can ensure there won't be any regressions.  Just
> do what I said above: Make sure the code never tries to acquire the
> global spinlock while already holding one of the per-port spinlocks.
> 
>> We mark gser->ioport as null pointer in gserial_disconnect, and in
>> gserial_resume we copy the gser->ioport to *port in the beginning.
>>
>> struct gs_port *port = gser->ioport;
>>
>> And hence it wont cause null pointer deref after the check as we don't
>> de-reference anything from gser->ioport afterwards. We only use the local
>> pointer *port afterwards.
> 
> You cannot depend on this to work the way you want.  The compiler will
> optimize your source code, and one of the optimizations might be to
> eliminate the "port" variable entirely and replace it with gser->ioport.
> 
> Alan Stern
Hi Alan, Thanks for the detailed info. I checked and included few cases 
here.

This would cause a deadlock if gserial_disconnect acquires port_lock and 
gserial_resume acquires static_lock.

gserial_disconnect {
	spin_lock(port)
	...
	spin_lock(static)

	gser->ioport = NULL;

	spin_unlock(static)
	...
	spin_unlock(port)
}

gserial_resume {
	struct gs_port *port = gser->ioport;

	spin_lock(static)
	if (!port)
		return
	spin_lock(port)
	spin_unlock(static)

	...
	spin_unlock(port)
}

------------------------------------------------------------------

This would cause additional races when gserial_disconnect releases 
port_lock and some other functions acquire it.

gserial_disconnect {
	spin_lock(port)
	...
	spin_unlock(port)
	spin_lock(static)

	gser->ioport = NULL;

	spin_unlock(static)
	spin_lock(port)
	...
	spin_unlock(port)
}

gserial_resume {
	struct gs_port *port = gser->ioport;

	spin_lock(static)
	if (!port)
		return
	spin_lock(port)
	spin_unlock(static)

	...
	spin_unlock(port)
}

------------------------------------------------------------------

And this seems like a viable option to me, what do you suggest?

gserial_disconnect {
	spin_lock(static)
	spin_lock(port)
	...
	gser->ioport = NULL;
	...	
	spin_lock(port)
	spin_unlock(static)

}

gserial_resume {
	struct gs_port *port = gser->ioport;

	spin_lock(static)
	if (!port)
		return
	spin_lock(port)

	...
	spin_unlock(port)
	spin_unlock(static)
}

Thanks,
Prashanth K
  

> And this seems like a viable option to me, what do you suggest?
> 
> gserial_disconnect {
>      spin_lock(static)
>      spin_lock(port)
>      ...
>      gser->ioport = NULL;
>      ...
>      spin_lock(port)
>      spin_unlock(static)
> 
> }
> 
> gserial_resume {
>      struct gs_port *port = gser->ioport;
> 
>      spin_lock(static)
>      if (!port)
	   spin_unlock(static)
>          return
>      spin_lock(port)
> 
>      ...
>      spin_unlock(port)
>      spin_unlock(static)
> }
> 
> Thanks,
> Prashanth K
Small correction inlined.
  

On Fri, Feb 10, 2023 at 12:26:52PM +0530, Prashanth K wrote:
> 
> 
> > And this seems like a viable option to me, what do you suggest?
> > 
> > gserial_disconnect {
> >      spin_lock(static)
> >      spin_lock(port)
> >      ...
> >      gser->ioport = NULL;
> >      ...
> >      spin_lock(port)
> >      spin_unlock(static)
> > 
> > }
> > 
> > gserial_resume {
> >      struct gs_port *port = gser->ioport;
> > 
> >      spin_lock(static)
> >      if (!port)
> 	   spin_unlock(static)
> >          return
> >      spin_lock(port)

If you want, you could move the spin_unlock(static) up to here.  It 
probably doesn't matter.

> > 
> >      ...
> >      spin_unlock(port)
> >      spin_unlock(static)
> > }

I agree, that should work fine.

Alan Stern
  

On 10-02-23 09:17 pm, Alan Stern wrote:
> 
> I agree, that should work fine.
> 
> Alan Stern
Thanks Alan for the help! will create v2 patch.