Re: [PATCH net v3 05/14] net: pse-pd: do not reach a freed PHY from the notification worker

[Kory Maincent]

> The notification worker holds a reference on the pse_control, but not
> on the PHY that psec->attached_phydev points to. If the PHY is removed
> while the worker runs, the pse_control survives and the worker reads the
> freed PHY. The same happens without a race when two PHYs share one PI:
> attached_phydev keeps pointing at the first PHY after it is removed.
> 
> pse_control_put() unlinks and frees a pse_control under pse_list_mutex,
> and phy_device_remove() calls it before the PHY is freed. So instead of
> taking a reference, let the worker look the pse_control up and use
> attached_phydev under pse_list_mutex. This nests pse_list_mutex inside
> rtnl; nothing under pse_list_mutex takes rtnl.
> 
> For a PI shared by two PHYs there is no single netdev to report to, so
> clear attached_phydev when a second PHY takes the pse_control, and warn.
> Events for that PI are then no longer reported, even after one PHY goes
> away. Failing the second PHY instead would fail its whole MDIO bus.
> 
> Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
> removing a PHY while the worker waits for rtnl, and removing one of
> two PHYs on a shared PI, both give
> 
>   BUG: KASAN: slab-use-after-free in pse_send_ntf_worker+0x2b0/0x2bc
> 
> With this patch both are gone.
> 
> Fixes: fc0e6db30941 ("net: pse-pd: Add support for reporting events")
> Acked-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> Signed-off-by: Carlo Szelinsky <github@xxxxxxxxxxxx>
>
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index eab7e25f2039b..af9eb29792706 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -238,24 +238,22 @@ static int of_load_pse_pis(struct pse_controller_dev *pcdev)
>   * @pcdev: a pointer to the PSE
>   * @id: index of the PSE control
>   *
> - * Return: pse_control pointer or NULL. The device returned has had a
> - *	   reference added and the pointer is safe until the user calls
> - *	   pse_control_put() to indicate they have finished with it.
> + * Must be called with pse_list_mutex held. No reference is taken: the
> + * pointer is only valid until pse_list_mutex is released.
> + *
> + * Return: pse_control pointer or NULL.
>   */
>  static struct pse_control *
>  pse_control_find_by_id(struct pse_controller_dev *pcdev, int id)
>  {
>  	struct pse_control *psec;
>  
> -	mutex_lock(&pse_list_mutex);
> +	lockdep_assert_held(&pse_list_mutex);
> +
>  	list_for_each_entry(psec, &pcdev->pse_control_head, list) {
> -		if (psec->id == id) {
> -			kref_get(&psec->refcnt);
> -			mutex_unlock(&pse_list_mutex);
> +		if (psec->id == id)
>  			return psec;
> -		}
>  	}
> -	mutex_unlock(&pse_list_mutex);
>  	return NULL;
>  }
>  
> @@ -268,6 +266,7 @@ pse_control_find_by_id(struct pse_controller_dev *pcdev, int id)
>  static struct net_device *pse_control_get_netdev(struct pse_control *psec)
>  {
>  	ASSERT_RTNL();
> +	lockdep_assert_held(&pse_list_mutex);
>  
>  	if (!psec || !psec->attached_phydev)
>  		return NULL;
> @@ -1015,13 +1014,23 @@ static void pse_send_ntf_worker(struct work_struct *work)
>  		struct net_device *netdev;
>  		struct pse_control *psec;
>  
> -		psec = pse_control_find_by_id(pcdev, ntf.id);
> +		/* If attached_phydev is set, it names the only PHY that
> +		 * holds this pse_control: a second PHY on the same PI
> +		 * clears it in pse_control_get_internal(). That PHY's
> +		 * pse_control_put() unlinks and frees the pse_control under
> +		 * pse_list_mutex before the PHY itself can be freed. Holding
> +		 * the mutex across the lookup and the attached_phydev
> +		 * dereference therefore keeps the PHY alive, which a
> +		 * reference on the pse_control alone would not.
> +		 */

Can you summarize this comment. It's hard to read.
Also we have the commit message that explain the why.

>  		rtnl_lock();
> +		mutex_lock(&pse_list_mutex);
> +		psec = pse_control_find_by_id(pcdev, ntf.id);
>  		netdev = pse_control_get_netdev(psec);
>  		if (netdev)
>  			ethnl_pse_send_ntf(netdev, ntf.notifs);
> +		mutex_unlock(&pse_list_mutex);
>  		rtnl_unlock();
> -		pse_control_put(psec);
>  	}
>  }
>  
> @@ -1412,6 +1421,19 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
>  
>  	list_for_each_entry(psec, &pcdev->pse_control_head, list) {
>  		if (psec->id == index) {
> +			/* attached_phydev can only name one PHY. If a second
> +			 * one shares the PI, either may be removed first and
> +			 * leave it pointing at a freed phy_device, so stop
> +			 * reporting events for this PI instead.
> +			 */

Same here. The warn message explain already the issue.

With these changes you can add my Reviewed-by.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com