netdev - Re: RFC: wilc1000: refactor TX path to use sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57d8cdfe5c22bf3df2727a18a6096026c59729da.camel@egauge.net>
Date:   Tue, 14 Dec 2021 10:43:35 -0700
From:   David Mosberger-Tang <davidm@...uge.net>
To:     Kalle Valo <kvalo@...nel.org>
Cc:     Ajay Singh <ajay.kathat@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: RFC: wilc1000: refactor TX path to use sk_buff queue

On Tue, 2021-12-14 at 19:36 +0200, Kalle Valo wrote:
> David Mosberger-Tang <davidm@...uge.net> writes:
> 
> > I'd like to propose to restructure the wilc1000 TX path to take
> > advantage of the existing sk_buff queuing and buffer operations rather
> > than using a driver-specific solution.  To me, the resulting code looks
> > simpler and the diffstat shows a fair amount of code-reduction:
> > 
> >  cfg80211.c |   35 ----
> >  mon.c      |   36 ----
> >  netdev.c   |   28 ---
> >  netdev.h   |   10 -
> >  wlan.c     |  499 ++++++++++++++++++++++++++-----------------------------------
> >  wlan.h     |   51 ++----
> >  6 files changed, 255 insertions(+), 404 deletions(-)
> 
> Looks like a very good cleanup.

Thanks!

> > +static void wilc_wlan_txq_drop_net_pkt(struct sk_buff *skb)
> > +{
> > +	struct wilc_vif *vif = netdev_priv(skb->dev);
> > +	struct wilc *wilc = vif->wilc;
> > +	struct wilc_skb_tx_cb *tx_cb = WILC_SKB_TX_CB(skb);
> > +
> > +	if ((u8)tx_cb->q_num >= NQUEUES) {
> > +		netdev_err(vif->ndev, "Invalid AC queue number %d",
> > +			   tx_cb->q_num);
> > +		return;
> > +	}
> 
> But why the cast here? Casting should be avoided as much as possible,
> they just create so many problems if used badly.

tx_cb->q_num is declared as:

       enum ip_pkt_priority q_num;     /* AC queue number */

so the cast to (u8) is to protect against negative values (which, of
course, should never really be the case).  Would you rather have the
code check explicitly for negative numbers, i.e.:

    if (tx_cb->q_num < 0 || tx_cb->q_num >= NQUEUES)

?

  --david