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

From: Xie He <xie.he.0141@gmail.com>
To: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, andrianov <andrianov@ispras.ru>,
	netdev <netdev@vger.kernel.org>
Subject: Re: Regarding possible bug in net/wan/x25_asy.c
Date: Sun, 23 Aug 2020 12:12:01 -0700	[thread overview]
Message-ID: <CAJht_EPrOuk3uweCNy06s0UQTBwkwCzjoS9fMfP8DMRAt8UV8w@mail.gmail.com> (raw)
In-Reply-To: <CAD=jOEY=8T3-USi63hy47BZKfTYcsUw-s-jAc3xi9ksk-je+XA@mail.gmail.com>

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