Regarding possible bug in net/wan/x25_asy.c

Regarding possible bug in net/wan/x25_asy.c

[Madhuparna Bhowmik]

Hello,

I have the following doubt:

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.

Thanks and regards,
Madhuparna

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

[Xie He]

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!

Thanks,
Xie He

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

[Madhuparna Bhowmik]

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

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

[Xie He]

On Mon, Aug 24, 2020 at 7:13 AM Madhuparna Bhowmik
<madhuparnabhowmik10@gmail.com> wrote:
>
> 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.

OK. Thank you for reporting!

I think the Linux driver verification project works very well because
it can help to find data races.

This driver might take a long time to fix because it has many issues,
and developers who are interested in it and are able to review patches
to it are rare.

Xie He

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

[Madhuparna Bhowmik]

On Mon, Aug 24, 2020 at 01:49:04PM -0700, Xie He wrote:
> On Mon, Aug 24, 2020 at 7:13 AM Madhuparna Bhowmik
> <madhuparnabhowmik10@gmail.com> wrote:
> >
> > 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.
> 
> OK. Thank you for reporting!
> 
> I think the Linux driver verification project works very well because
> it can help to find data races.
>
Yes, indeed!

> This driver might take a long time to fix because it has many issues,
> and developers who are interested in it and are able to review patches
> to it are rare.
>
Alright, hope it is fixed soon!

Thanks,
Madhuparna

> Xie He