Skip to content

Commit

Permalink
lan78xx: Fix race condition in disconnect handling
Browse files Browse the repository at this point in the history
If there is a device disconnect at roughly the same time as a
deferred PHY link reset there is a race condition that can result
in a kernel lock up due to a null pointer dereference in the
driver's deferred work handling routine lan78xx_delayedwork().
The following changes fix this problem.

Add new status flag EVENT_DEV_DISCONNECT to indicate when the
device has been removed and use it to prevent operations, such as
register access, that will fail once the device is removed.

Stop processing of deferred work items when the driver's USB
disconnect handler is invoked.

Disconnect the PHY only after the network device has been
unregistered and all delayed work has been cancelled.

Signed-off-by: John Efstathiades <john.efstathiades@pebblebay.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
John Efstathiades authored and David S. Miller committed Aug 25, 2021
1 parent 5f4cc6e commit 77dfff5
Showing 1 changed file with 57 additions and 9 deletions.
66 changes: 57 additions & 9 deletions drivers/net/usb/lan78xx.c
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ struct usb_context {
#define EVENT_DEV_ASLEEP 7
#define EVENT_DEV_OPEN 8
#define EVENT_STAT_UPDATE 9
#define EVENT_DEV_DISCONNECT 10

struct statstage {
struct mutex access_lock; /* for stats access */
Expand Down Expand Up @@ -444,9 +445,13 @@ MODULE_PARM_DESC(msg_level, "Override default message level");

static int lan78xx_read_reg(struct lan78xx_net *dev, u32 index, u32 *data)
{
u32 *buf = kmalloc(sizeof(u32), GFP_KERNEL);
u32 *buf;
int ret;

if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
return -ENODEV;

buf = kmalloc(sizeof(u32), GFP_KERNEL);
if (!buf)
return -ENOMEM;

Expand All @@ -470,9 +475,13 @@ static int lan78xx_read_reg(struct lan78xx_net *dev, u32 index, u32 *data)

static int lan78xx_write_reg(struct lan78xx_net *dev, u32 index, u32 data)
{
u32 *buf = kmalloc(sizeof(u32), GFP_KERNEL);
u32 *buf;
int ret;

if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
return -ENODEV;

buf = kmalloc(sizeof(u32), GFP_KERNEL);
if (!buf)
return -ENOMEM;

Expand Down Expand Up @@ -3146,16 +3155,23 @@ static void tx_complete(struct urb *urb)
/* software-driven interface shutdown */
case -ECONNRESET:
case -ESHUTDOWN:
netif_dbg(dev, tx_err, dev->net,
"tx err interface gone %d\n",
entry->urb->status);
break;

case -EPROTO:
case -ETIME:
case -EILSEQ:
netif_stop_queue(dev->net);
netif_dbg(dev, tx_err, dev->net,
"tx err queue stopped %d\n",
entry->urb->status);
break;
default:
netif_dbg(dev, tx_err, dev->net,
"tx err %d\n", entry->urb->status);
"unknown tx err %d\n",
entry->urb->status);
break;
}
}
Expand Down Expand Up @@ -3489,6 +3505,7 @@ static int rx_submit(struct lan78xx_net *dev, struct urb *urb, gfp_t flags)
lan78xx_defer_kevent(dev, EVENT_RX_HALT);
break;
case -ENODEV:
case -ENOENT:
netif_dbg(dev, ifdown, dev->net, "device gone\n");
netif_device_detach(dev->net);
break;
Expand Down Expand Up @@ -3689,6 +3706,12 @@ static void lan78xx_tx_bh(struct lan78xx_net *dev)
lan78xx_defer_kevent(dev, EVENT_TX_HALT);
usb_autopm_put_interface_async(dev->intf);
break;
case -ENODEV:
case -ENOENT:
netif_dbg(dev, tx_err, dev->net,
"tx: submit urb err %d (disconnected?)", ret);
netif_device_detach(dev->net);
break;
default:
usb_autopm_put_interface_async(dev->intf);
netif_dbg(dev, tx_err, dev->net,
Expand Down Expand Up @@ -3783,6 +3806,9 @@ static void lan78xx_delayedwork(struct work_struct *work)

dev = container_of(work, struct lan78xx_net, wq.work);

if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
return;

if (usb_autopm_get_interface(dev->intf) < 0)
return;

Expand Down Expand Up @@ -3857,6 +3883,7 @@ static void intr_complete(struct urb *urb)

/* software-driven interface shutdown */
case -ENOENT: /* urb killed */
case -ENODEV: /* hardware gone */
case -ESHUTDOWN: /* hardware gone */
netif_dbg(dev, ifdown, dev->net,
"intr shutdown, code %d\n", status);
Expand All @@ -3870,14 +3897,29 @@ static void intr_complete(struct urb *urb)
break;
}

if (!netif_running(dev->net))
if (!netif_device_present(dev->net) ||
!netif_running(dev->net)) {
netdev_warn(dev->net, "not submitting new status URB");
return;
}

memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
status = usb_submit_urb(urb, GFP_ATOMIC);
if (status != 0)

switch (status) {
case 0:
break;
case -ENODEV:
case -ENOENT:
netif_dbg(dev, timer, dev->net,
"intr resubmit %d (disconnect?)", status);
netif_device_detach(dev->net);
break;
default:
netif_err(dev, timer, dev->net,
"intr resubmit --> %d\n", status);
break;
}
}

static void lan78xx_disconnect(struct usb_interface *intf)
Expand All @@ -3892,8 +3934,15 @@ static void lan78xx_disconnect(struct usb_interface *intf)
if (!dev)
return;

set_bit(EVENT_DEV_DISCONNECT, &dev->flags);

udev = interface_to_usbdev(intf);
net = dev->net;

unregister_netdev(net);

cancel_delayed_work_sync(&dev->wq);

phydev = net->phydev;

phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
Expand All @@ -3904,12 +3953,11 @@ static void lan78xx_disconnect(struct usb_interface *intf)
if (phy_is_pseudo_fixed_link(phydev))
fixed_phy_unregister(phydev);

unregister_netdev(net);

cancel_delayed_work_sync(&dev->wq);

usb_scuttle_anchored_urbs(&dev->deferred);

if (timer_pending(&dev->stat_monitor))
del_timer_sync(&dev->stat_monitor);

lan78xx_unbind(dev, intf);

usb_kill_urb(dev->urb_intr);
Expand Down

0 comments on commit 77dfff5

Please sign in to comment.