Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 71B2AC433CA
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Vsevolod Kozlov <zaba@mm.st>
Cc:     ajay.kathat@microchip.com, claudiu.beznea@microchip.com,
        linux-wireless@vger.kernel.org
Subject: Re: [PATCH] wilc1000: Fix use of void pointer as a wrong struct type
References: <YB7gI+c1X633KOde@Vsevolods-Mini.lan>
Date:   Tue, 09 Feb 2021 09:56:13 +0200
In-Reply-To: <YB7gI+c1X633KOde@Vsevolods-Mini.lan> (Vsevolod Kozlov's message
        of "Sat, 6 Feb 2021 20:29:55 +0200")
Message-ID: <871rdpk736.fsf@tynnyri.adurom.net>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Vsevolod Kozlov <zaba@mm.st> writes:

> ac_classify() expects a struct sk_buff* as its second argument, which is
> a member of struct tx_complete_data. priv happens to be a pointer to
> struct tx_complete_data, so passing it directly to ac_classify() leads
> to wrong behaviour and occasional panics.

A perfect example why void pointers should be avoided.

> Signed-off-by: Vsevolod Kozlov <zaba@mm.st>
> ---
>  drivers/net/wireless/microchip/wilc1000/wlan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index c12f27be9f79..04ed52c736ff 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -415,6 +415,7 @@ int wilc_wlan_txq_add_net_pkt(struct net_device *dev, void *priv, u8 *buffer,
>  	struct txq_entry_t *tqe;
>  	struct wilc_vif *vif = netdev_priv(dev);
>  	struct wilc *wilc;
> +	struct tx_complete_data *tx_data = priv;
>  	u8 q_num;
>  
>  	wilc = vif->wilc;
> @@ -437,7 +438,7 @@ int wilc_wlan_txq_add_net_pkt(struct net_device *dev, void *priv, u8 *buffer,
>  	tqe->priv = priv;
>  	tqe->vif = vif;
>  
> -	q_num = ac_classify(wilc, priv);
> +	q_num = ac_classify(wilc, tx_data->skb);

I think a safer fix would be to change wilc_wlan_txq_add_net_pkt() to
take that struct tx_complete_data *tx_data directly, and not use void
pointer at all. At the same time you could remove the ugly cast from the
caller:

netdev.c:740:   queue_count = wilc_wlan_txq_add_net_pkt(ndev, (void *)tx_data,

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches