Received-SPF: pass (google.com: domain of linux-kernel+bounces-111876-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Sai Krishna Gajula <saikrishnag@marvell.com>
To: Oliver Neukum <oneukum@suse.com>,
        "davem@davemloft.net"
	<davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC: "syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com"
	<syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com>
Subject: RE:  [PATCH net-next] usbnet: fix cyclical race on disconnect with
 work queue
Thread-Topic: [PATCH net-next] usbnet: fix cyclical race on disconnect with
 work queue
Thread-Index: AQHafIBp1dqMXcvK9kC5zLoffE3WoA==
Date: Fri, 22 Mar 2024 17:43:12 +0000
Message-ID: 
 <SA1PR18MB470955BBB332D3A9F9A6F247A0312@SA1PR18MB4709.namprd18.prod.outlook.com>
References: <20240321124758.6302-1-oneukum@suse.com>
In-Reply-To: <20240321124758.6302-1-oneukum@suse.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?sKcKHbbTmwPGb2HZ1u6sPqsh34wFGNvzbGd2DuKeOico+NWMDyNJTRKMRq2w?=
 =?us-ascii?Q?QYZ5r9qeRlPHg0oypLfu8MO+3Ltsg/zSDEpeYRr0vGqGSN/ux82E9s2Gavrs?=
 =?us-ascii?Q?NywtjQ2fCH+w6XXnW1bxuCtVWqAB4yo4Oj/Mpdvzx0cJgfKVfFy+/qYMTdmT?=
 =?us-ascii?Q?lDgOX5WBC1UnUpiColVQjnDj+ga10r2poR5BSMVws0BdqRZf8tv+W1qreyLQ?=
 =?us-ascii?Q?uT4v7Ze9S9B5bpwCMqLggKvWgaS/bDDltrUe4aQyv/vgRt7iFTCAZ4pK1l+v?=
 =?us-ascii?Q?P+CEF0pJuSK1faZaTwsY2AQ03uCZ0lrzKKgnzUXBid7Le9EY5CSfmPQrOqdM?=
 =?us-ascii?Q?RqxoubHrl7Ww7IamzkQolr3xQKJfz5R2AtOR4rEYIw1bmWdTxUjAXNoL6C1D?=
 =?us-ascii?Q?iqye0oUVhhFnMBpwSBYbn98Y/25eoUvtSPyXGR+YMRorlZiGv7xzsz199Xuh?=
 =?us-ascii?Q?38gZtJimsfOx9JWYV45U4jBAD4+Y9A3Fosg1qYs8D7bSceU6ph+c1O9S4GVE?=
 =?us-ascii?Q?V5DMtjfnVyLuRTz583OUK0rpzGgtTc6t6pv8SM7+7+CfLz+gTC0brCfS0qJh?=
 =?us-ascii?Q?iMKWCI6UM391FwURzOKGElJXkFp4Y3d87x3todWtYHUL/OcCFvH7TwvJLKVT?=
 =?us-ascii?Q?eYfPst6V/HnxiHM3Kif7lgQu/P1I7/GgnofiXGCiUa9lS+7DIPNzgUPDtmAe?=
 =?us-ascii?Q?eLzEQU8tTzoVyfamu4EP/lkC3KxdsrvD0hOJccN4Ket7rQ8DWu3QV1h/G8QS?=
 =?us-ascii?Q?QUmp04UYALAm7+EoKBzotrMOL6Pw3iHQAlGg71A7JoCtEtOjJ/qHFwiJDrxW?=
 =?us-ascii?Q?HiEyiSyh/Ll5AuGaPd0uvxQVlmMfoQCSvVVNo5hi9XFzwLaSQy+RuJu7THpM?=
 =?us-ascii?Q?ziOYXjZ2yssZUuCtZnsgrK5qcWXqbx6SHHtVtTMXjr5zvoWcwYNOmFaMGoo9?=
 =?us-ascii?Q?CRDjJxKBkT+Ct1+epbH9i/q4zYMNZkcnGDCGieP0adxSpfbrzYBKiDCoK0Uv?=
 =?us-ascii?Q?x/CMB1wA0GE+7NL39qTZXDwwtFwJcIOeqZ68QeBbIc7z0WDMseFYOmozf8L0?=
 =?us-ascii?Q?7Fw/5KA5ESzshYMQYdbrhkAOkP9wPmkUpONuikIoN6Bvx3a7H9+uE4uO+TxO?=
 =?us-ascii?Q?t8efTMXgC5ijBGA8gXcr7VWNwydFiNNJbQnxeisiCbe+KcGkkeeDFzukmZNw?=
 =?us-ascii?Q?5r4kPYhgrQJtQwNPJU+Is0qOmBWInJcPD9JWrjBI2WXB014qJH/tico7EL7T?=
 =?us-ascii?Q?R52Y1z4SiR+IgUHx9DP7Z9l12L+f048miQnUUNIcOTgNmpTYTIVgk5TDISQt?=
 =?us-ascii?Q?tmvUpLuKtQqAQSz3wTnr9opb3y5ei39USl9He0jKcQQik/4X6NkEsgWhcBNy?=
 =?us-ascii?Q?UuBO/0VmpnTXiGoMrSySomIYuX+UeXcr5qPxpvoF8On35r4kXxgN5aWhcS+C?=
 =?us-ascii?Q?3UJuzskYRo8KLiWUjqzsAvoUBk7jjCMVUgRKHC8yf8vmg92zVuR6L3RTz67j?=
 =?us-ascii?Q?anjRJ7cQ1Xj0LIrkXcbubToqwyaCFb942DXW5TtjTF6jg7hu7q9SxxNGWQSM?=
 =?us-ascii?Q?0jr8vSqPjTsDAWVv17eza8YxTwtu6CMtD6ti4Qy/?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR18MB4709.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 881dd50b-085f-4afc-9124-08dc4a978be5
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Mar 2024 17:43:12.7229
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: zZ59++4ndPBVD+WiyuinZc0gH+cF9bsON3i0nykYCJscwXevvshbrwPmc0u9voeAjUClYjGM8lFr2ElkUNonHA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR18MB5994
X-Proofpoint-GUID: 5MOq8SKb9LkO3prPFPHgEf3QCA8V7NjH
X-Proofpoint-ORIG-GUID: 5MOq8SKb9LkO3prPFPHgEf3QCA8V7NjH
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-22_10,2024-03-21_02,2023-05-22_02


> -----Original Message-----
> From: Oliver Neukum <oneukum@suse.com>
> Sent: Thursday, March 21, 2024 6:17 PM
> To: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Oliver Neukum <oneukum@suse.com>;
> syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com
> Subject: [PATCH net-next] usbnet: fix cyclical race on disconnect
> with work queue

This patch seems to be a fix, in that case the subject need to be with [PAT=
CH net]

>=20
> The work can submit URBs and the URBs can schedule the work.
> This cycle needs to be broken, when a device is to be stopped.
> Use a flag to do so.
>=20
> Fixes: f29fc259976e9 ("[PATCH] USB: usbnet (1/9) clean up framing")

Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")=
' - ie: 'Fixes: f29fc259976e ("[PATCH] USB: usbnet (1/9) clean up framing")=
'=20

> Reported-by: syzbot+9665bf55b1c828bbcd8a@syzkaller.appspotmail.com
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/usbnet.c   | 37 ++++++++++++++++++++++++++++---------
>  include/linux/usb/usbnet.h | 18 ++++++++++++++++++
>  2 files changed, 46 insertions(+), 9 deletions(-)
>=20
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index
> e84efa661589..422d91635045 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -467,10 +467,12 @@ static enum skb_state defer_bh(struct usbnet *dev,
> struct sk_buff *skb,  void usbnet_defer_kevent (struct usbnet *dev, int w=
ork)

space prohibited between function name and open parenthesis '('

> {
>  	set_bit (work, &dev->flags);
> -	if (!schedule_work (&dev->kevent))
> -		netdev_dbg(dev->net, "kevent %s may have been
> dropped\n", usbnet_event_names[work]);
> -	else
> -		netdev_dbg(dev->net, "kevent %s scheduled\n",
> usbnet_event_names[work]);
> +	if (!usbnet_going_away(dev)) {
> +		if (!schedule_work (&dev->kevent))
> +			netdev_dbg(dev->net, "kevent %s may have been
> dropped\n", usbnet_event_names[work]);
> +		else
> +			netdev_dbg(dev->net, "kevent %s scheduled\n",
> usbnet_event_names[work]);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
>=20
> @@ -538,7 +540,8 @@ static int rx_submit (struct usbnet *dev, struct urb
> *urb, gfp_t flags)

space prohibited between function name and open parenthesis '(', where ever=
 applicable.

>  			tasklet_schedule (&dev->bh);
>  			break;
>  		case 0:
> -			__usbnet_queue_skb(&dev->rxq, skb, rx_start);
> +			if (!usbnet_going_away(dev))
> +				__usbnet_queue_skb(&dev->rxq, skb,
> rx_start);
>  		}
>  	} else {
>  		netif_dbg(dev, ifdown, dev->net, "rx: stopped\n"); @@ -849,6
> +852,16 @@ int usbnet_stop (struct net_device *net)
>  	del_timer_sync (&dev->delay);
>  	tasklet_kill (&dev->bh);
>  	cancel_work_sync(&dev->kevent);
> +
> +	/*
> +	 * we have cyclic dependencies. Those calls are needed
> +	 * to break a cycle. We cannot fall into the gaps because
> +	 * we have a flag
> +	 */

Networking block comments don't use an empty /* line, use /* Comment...

> +	tasklet_kill (&dev->bh);
> +	del_timer_sync (&dev->delay);
> +	cancel_work_sync(&dev->kevent);
> +
>  	if (!pm)
>  		usb_autopm_put_interface(dev->intf);
>=20
> @@ -1174,7 +1187,8 @@ usbnet_deferred_kevent (struct work_struct *work)
>  					   status);
>  		} else {
>  			clear_bit (EVENT_RX_HALT, &dev->flags);
> -			tasklet_schedule (&dev->bh);
> +			if (!usbnet_going_away(dev))
> +				tasklet_schedule (&dev->bh);
>  		}
>  	}
>=20
> @@ -1196,10 +1210,13 @@ usbnet_deferred_kevent (struct work_struct
> *work)
>  			}
>  			if (rx_submit (dev, urb, GFP_KERNEL) =3D=3D -ENOLINK)
>  				resched =3D 0;
> -			usb_autopm_put_interface(dev->intf);
>  fail_lowmem:
> -			if (resched)
> +			usb_autopm_put_interface(dev->intf);
> +			if (resched) {
> +				set_bit (EVENT_RX_MEMORY, &dev->flags);
> +
>  				tasklet_schedule (&dev->bh);
> +			}
>  		}
>  	}
>=20
> @@ -1212,13 +1229,13 @@ usbnet_deferred_kevent (struct work_struct
> *work)
>  		if (status < 0)
>  			goto skip_reset;
>  		if(info->link_reset && (retval =3D info->link_reset(dev)) < 0) {
> -			usb_autopm_put_interface(dev->intf);
>  skip_reset:
>  			netdev_info(dev->net, "link reset failed (%d) usbnet
> usb-%s-%s, %s\n",
>  				    retval,
>  				    dev->udev->bus->bus_name,
>  				    dev->udev->devpath,
>  				    info->description);
> +			usb_autopm_put_interface(dev->intf);
>  		} else {
>  			usb_autopm_put_interface(dev->intf);
>  		}
> @@ -1562,6 +1579,7 @@ static void usbnet_bh (struct timer_list *t)
>  	} else if (netif_running (dev->net) &&
>  		   netif_device_present (dev->net) &&
>  		   netif_carrier_ok(dev->net) &&
> +		   !usbnet_going_away(dev) &&
>  		   !timer_pending(&dev->delay) &&
>  		   !test_bit(EVENT_RX_PAUSED, &dev->flags) &&
>  		   !test_bit(EVENT_RX_HALT, &dev->flags)) { @@ -1609,6
> +1627,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>  	usb_set_intfdata(intf, NULL);
>  	if (!dev)
>  		return;
> +	usbnet_mark_going_away(dev);
>=20
>  	xdev =3D interface_to_usbdev (intf);
>=20
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h inde=
x
> 9f08a584d707..d26599faab33 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -76,8 +76,26 @@ struct usbnet {
>  #		define EVENT_LINK_CHANGE	11
>  #		define EVENT_SET_RX_MODE	12
>  #		define EVENT_NO_IP_ALIGN	13
> +/*
> + * this one is special, as it indicates that the device is going away
> + * there are cyclic dependencies between tasklet, timer and bh
> + * that must be broken
> + */

Networking block comments don't use an empty /* line, use /* Comment...

> +#		define EVENT_UNPLUG		31
>  };
>=20
> +static inline bool usbnet_going_away(struct usbnet *ubn) {
> +	smp_mb__before_atomic();
> +	return test_bit(EVENT_UNPLUG, &ubn->flags); }
> +
> +static inline void usbnet_mark_going_away(struct usbnet *ubn) {
> +	set_bit(EVENT_UNPLUG, &ubn->flags);
> +	smp_mb__after_atomic();
> +}
> +
>  static inline struct usb_driver *driver_of(struct usb_interface *intf)  =
{
>  	return to_usb_driver(intf->dev.driver);
> --
> 2.44.0
>=20