Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* Regarding possible bug in net/wan/x25_asy.c
@ 2020-08-23 15:28 Madhuparna Bhowmik
  2020-08-23 19:12 ` Xie He
  0 siblings, 1 reply; 5+ messages in thread
From: Madhuparna Bhowmik @ 2020-08-23 15:28 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, xie.he.0141; +Cc: andrianov, netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Regarding possible bug in net/wan/x25_asy.c
  2020-08-23 15:28 Regarding possible bug in net/wan/x25_asy.c Madhuparna Bhowmik
@ 2020-08-23 19:12 ` Xie He
  2020-08-24 14:13   ` Madhuparna Bhowmik
  0 siblings, 1 reply; 5+ messages in thread
From: Xie He @ 2020-08-23 19:12 UTC (permalink / raw)
  To: Madhuparna Bhowmik; +Cc: David Miller, Jakub Kicinski, andrianov, netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Regarding possible bug in net/wan/x25_asy.c
  2020-08-23 19:12 ` Xie He
@ 2020-08-24 14:13   ` Madhuparna Bhowmik
  2020-08-24 20:49     ` Xie He
  0 siblings, 1 reply; 5+ messages in thread
From: Madhuparna Bhowmik @ 2020-08-24 14:13 UTC (permalink / raw)
  To: Xie He
  Cc: Madhuparna Bhowmik, David Miller, Jakub Kicinski, andrianov, netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Regarding possible bug in net/wan/x25_asy.c
  2020-08-24 14:13   ` Madhuparna Bhowmik
@ 2020-08-24 20:49     ` Xie He
  2020-08-26 14:35       ` Madhuparna Bhowmik
  0 siblings, 1 reply; 5+ messages in thread
From: Xie He @ 2020-08-24 20:49 UTC (permalink / raw)
  To: Madhuparna Bhowmik; +Cc: David Miller, Jakub Kicinski, andrianov, netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Regarding possible bug in net/wan/x25_asy.c
  2020-08-24 20:49     ` Xie He
@ 2020-08-26 14:35       ` Madhuparna Bhowmik
  0 siblings, 0 replies; 5+ messages in thread
From: Madhuparna Bhowmik @ 2020-08-26 14:35 UTC (permalink / raw)
  To: Xie He
  Cc: Madhuparna Bhowmik, David Miller, Jakub Kicinski, andrianov, netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-08-26 14:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-23 15:28 Regarding possible bug in net/wan/x25_asy.c Madhuparna Bhowmik
2020-08-23 19:12 ` Xie He
2020-08-24 14:13   ` Madhuparna Bhowmik
2020-08-24 20:49     ` Xie He
2020-08-26 14:35       ` Madhuparna Bhowmik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).