| Message ID | 20200926205610.21045-1-xie.he.0141@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] drivers/net/wan/x25_asy: Keep the ldisc running even when netif is down | expand |
From: Xie He <xie.he.0141@gmail.com> Date: Sat, 26 Sep 2020 13:56:10 -0700 > @@ -265,7 +269,9 @@ static void x25_asy_write_wakeup(struct tty_struct *tty) > * transmission of another packet */ > sl->dev->stats.tx_packets++; > clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); > - x25_asy_unlock(sl); > + /* FIXME: The netif may go down after netif_running returns */ > + if (netif_running(sl->dev)) > + x25_asy_unlock(sl); > return; It could also go back down and also back up again after you do this test. Maybe even 10 or 100 times over. You can't just leave things so incredibly racy like this, please apply proper synchronization between netdev state changes and this TTY code. Thank you.
On Mon, Sep 28, 2020 at 3:58 PM David Miller <davem@davemloft.net> wrote: > > It could also go back down and also back up again after you do this > test. Maybe even 10 or 100 times over. > > You can't just leave things so incredibly racy like this, please apply > proper synchronization between netdev state changes and this TTY code. > > Thank you. OK! Thanks! I'll try to ensure proper locking for the netif_running checks and re-submit.
diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c index c418767a890a..22fcc0dd4b57 100644 --- a/drivers/net/wan/x25_asy.c +++ b/drivers/net/wan/x25_asy.c @@ -192,6 +192,10 @@ static void x25_asy_bump(struct x25_asy *sl) int count; int err; + /* FIXME: The netif may go down after netif_running returns true */ + if (!netif_running(dev)) + return; + count = sl->rcount; dev->stats.rx_bytes += count; @@ -257,7 +261,7 @@ static void x25_asy_write_wakeup(struct tty_struct *tty) struct x25_asy *sl = tty->disc_data; /* First make sure we're connected. */ - if (!sl || sl->magic != X25_ASY_MAGIC || !netif_running(sl->dev)) + if (!sl || sl->magic != X25_ASY_MAGIC) return; if (sl->xleft <= 0) { @@ -265,7 +269,9 @@ static void x25_asy_write_wakeup(struct tty_struct *tty) * transmission of another packet */ sl->dev->stats.tx_packets++; clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); - x25_asy_unlock(sl); + /* FIXME: The netif may go down after netif_running returns */ + if (netif_running(sl->dev)) + x25_asy_unlock(sl); return; } @@ -529,7 +535,7 @@ static void x25_asy_receive_buf(struct tty_struct *tty, { struct x25_asy *sl = tty->disc_data; - if (!sl || sl->magic != X25_ASY_MAGIC || !netif_running(sl->dev)) + if (!sl || sl->magic != X25_ASY_MAGIC) return; @@ -605,6 +611,7 @@ static void x25_asy_close_tty(struct tty_struct *tty) dev_close(sl->dev); rtnl_unlock(); + x25_asy_close(sl->dev); tty->disc_data = NULL; sl->tty = NULL; x25_asy_free(sl); @@ -732,8 +739,6 @@ static int x25_asy_close_dev(struct net_device *dev) pr_err("%s: lapb_unregister error: %d\n", __func__, err); - x25_asy_close(dev); - return 0; }
I believe it is necessary to keep the N_X25 line discipline running even when the upper network interface is down, because otherwise, the last frame transmitted before the interface going down may be incompletely transmitted, and the first frame received after the interface going up may be incompletely received. By allowing the line discipline to continue doing R/W on the TTY, we can ensure that the last frame before the interface goes down is completely transmitted, and the first frame after the interface goes up is completely received. To achieve this, I did these changes: 1. Postpone the netif_running check in x25_asy_write_wakeup until the transmission of data is complete. 2. Postpone the netif_running check in x25_asy_receive_buf until a complete frame is received (in x25_asy_bump). 3. Move x25_asy_close from x25_asy_close_dev to x25_asy_close_tty, so that when closing the interface, TTY transmission will not stop and the line discipline's read buffer and write buffer will not be cleared. (Do these only when the line discipline is closing.) (Also add FIXME comments because the netif_running checks may race with the closing of the netif. Currently there's no locking to prevent this. This needs to be fixed.) Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/x25_asy.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)