Messages in this thread | | | Date | Fri, 10 Feb 2023 11:52:23 +0530 | Subject | Re: [PATCH] usb: gadget: u_serial: Add null pointer check in gserial_resume | From | Prashanth K <> |
| |
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
| |