Re: Regarding possible bug in net/wan/x25_asy.c

[Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>]

On Sun, Aug 23, 2020 at 12:12:01PM -0700, Xie He wrote:
> On Sun, Aug 23, 2020 at 8:28 AM Madhuparna Bhowmik
> <madhuparnabhowmik10@gmail.com> wrote:
> >
> > sl->xhead is modified in both x25_asy_change_mtu() and
> > x25_asy_write_wakeup(). However, sl->lock is not held in
> > x25_asy_write_wakeup(). So, I am not sure if it is indeed possible to
> > have a race between these two functions. If it is possible that these
> > two functions can execute in parallel then the lock should be held in
> > x25_asy_write_wakeup() as well. Please let me know if this race is
> > possible.
> 
> I think you are right. These two functions do race with each other.
> There seems to be nothing preventing them from racing. We need to hold
> the lock in x25_asy_write_wakeup to prevent it from racing with
> x25_asy_change_mtu.
> 
> By the way, I think this driver has bigger problems. We can see that
> these function pairs are not symmetric with one another in what they
> do:
> "x25_asy_alloc" and "x25_asy_free";
> "x25_asy_open" and "x25_asy_close";
> "x25_asy_open_tty" and "x25_asy_close_tty";
> "x25_asy_netdev_ops.ndo_open" and "x25_asy_netdev_ops.ndo_stop".
> 
> This not only makes the code messy, but also makes the actual runtime
> behavior buggy.
> 
> I'm planning to fix this with this change:
> https://github.com/hyanggi/linux/commit/66387f229168014024117d50ade01092e3c9932c
> Please take a look if you are interested. Thanks!
>
Sure, I had a look at it and since you are already working on fixing
this driver, don't think there is a need for a patch to fix the
particular race condition bug. This bug was found by the Linux driver
verification project and my work was to report it to the maintainers.

Thanks and Regards,
Madhuparna


> Thanks,
> Xie He