Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Jun 2023 14:33:41 +0200
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Fedor Pchelkin <pchelkin@ispras.ru>
Cc:     Oleksij Rempel <linux@rempel-privat.de>,
        Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>,
        lvc-project@linuxtesting.org,
        Robin van der Gracht <robin@protonic.nl>,
        linux-can@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        netdev@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        kernel@pengutronix.de, Oliver Hartkopp <socketcan@hartkopp.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] can: j1939: change j1939_netdev_lock type to mutex
Message-ID: <20230602123341.GG17237@pengutronix.de>
References: <20230526171910.227615-1-pchelkin@ispras.ru>
 <20230526171910.227615-2-pchelkin@ispras.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230526171910.227615-2-pchelkin@ispras.ru>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Fri, May 26, 2023 at 08:19:09PM +0300, Fedor Pchelkin wrote:
> It turns out access to j1939_can_rx_register() needs to be serialized,
> otherwise j1939_priv can be corrupted when parallel threads call
> j1939_netdev_start() and j1939_can_rx_register() fails. This issue is
> thoroughly covered in other commit which serializes access to
> j1939_can_rx_register().
> 
> Change j1939_netdev_lock type to mutex so that we do not need to remove
> GFP_KERNEL from can_rx_register().
> 
> j1939_netdev_lock seems to be used in normal contexts where mutex usage
> is not prohibited.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Suggested-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

> ---
> Note that it has been only tested via Syzkaller and not with real
> hardware.
> 
>  net/can/j1939/main.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 821d4ff303b3..6ed79afe19a5 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -126,7 +126,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data)
>  #define J1939_CAN_ID CAN_EFF_FLAG
>  #define J1939_CAN_MASK (CAN_EFF_FLAG | CAN_RTR_FLAG)
>  
> -static DEFINE_SPINLOCK(j1939_netdev_lock);
> +static DEFINE_MUTEX(j1939_netdev_lock);
>  
>  static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
>  {
> @@ -220,7 +220,7 @@ static void __j1939_rx_release(struct kref *kref)
>  	j1939_can_rx_unregister(priv);
>  	j1939_ecu_unmap_all(priv);
>  	j1939_priv_set(priv->ndev, NULL);
> -	spin_unlock(&j1939_netdev_lock);
> +	mutex_unlock(&j1939_netdev_lock);
>  }
>  
>  /* get pointer to priv without increasing ref counter */
> @@ -248,9 +248,9 @@ static struct j1939_priv *j1939_priv_get_by_ndev(struct net_device *ndev)
>  {
>  	struct j1939_priv *priv;
>  
> -	spin_lock(&j1939_netdev_lock);
> +	mutex_lock(&j1939_netdev_lock);
>  	priv = j1939_priv_get_by_ndev_locked(ndev);
> -	spin_unlock(&j1939_netdev_lock);
> +	mutex_unlock(&j1939_netdev_lock);
>  
>  	return priv;
>  }
> @@ -260,14 +260,14 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  	struct j1939_priv *priv, *priv_new;
>  	int ret;
>  
> -	spin_lock(&j1939_netdev_lock);
> +	mutex_lock(&j1939_netdev_lock);
>  	priv = j1939_priv_get_by_ndev_locked(ndev);
>  	if (priv) {
>  		kref_get(&priv->rx_kref);
> -		spin_unlock(&j1939_netdev_lock);
> +		mutex_unlock(&j1939_netdev_lock);
>  		return priv;
>  	}
> -	spin_unlock(&j1939_netdev_lock);
> +	mutex_unlock(&j1939_netdev_lock);
>  
>  	priv = j1939_priv_create(ndev);
>  	if (!priv)
> @@ -277,20 +277,20 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  	spin_lock_init(&priv->j1939_socks_lock);
>  	INIT_LIST_HEAD(&priv->j1939_socks);
>  
> -	spin_lock(&j1939_netdev_lock);
> +	mutex_lock(&j1939_netdev_lock);
>  	priv_new = j1939_priv_get_by_ndev_locked(ndev);
>  	if (priv_new) {
>  		/* Someone was faster than us, use their priv and roll
>  		 * back our's.
>  		 */
>  		kref_get(&priv_new->rx_kref);
> -		spin_unlock(&j1939_netdev_lock);
> +		mutex_unlock(&j1939_netdev_lock);
>  		dev_put(ndev);
>  		kfree(priv);
>  		return priv_new;
>  	}
>  	j1939_priv_set(ndev, priv);
> -	spin_unlock(&j1939_netdev_lock);
> +	mutex_unlock(&j1939_netdev_lock);
>  
>  	ret = j1939_can_rx_register(priv);
>  	if (ret < 0)
> @@ -308,7 +308,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  
>  void j1939_netdev_stop(struct j1939_priv *priv)
>  {
> -	kref_put_lock(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);
> +	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);
>  	j1939_priv_put(priv);
>  }
>  
> -- 
> 2.34.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |