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;
MIME-Version: 1.0
References: <20220912214559.929186-1-nhuck@google.com> <YyENqW9u3LxySbSk@dev-arch.thelio-3990X>
In-Reply-To: <YyENqW9u3LxySbSk@dev-arch.thelio-3990X>
From:   Nathan Huckleberry <nhuck@google.com>
Date:   Tue, 13 Sep 2022 18:45:00 -0700
Message-ID: <CAJkfWY6FYCNz_5+5cM2GoJdGLdBu7aPzs0e8uFmhgZRE_axLSQ@mail.gmail.com>
Subject: Re: [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit
To:     Nathan Chancellor <nathan@kernel.org>
Cc:     Dan Carpenter <error27@gmail.com>, llvm@lists.linux.dev,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Nathan,

On Tue, Sep 13, 2022 at 4:09 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Sep 12, 2022 at 02:45:56PM -0700, Nathan Huckleberry wrote:
> > The ndo_start_xmit field in net_device_ops is expected to be of type
> > netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
> >
> > The mismatched return type breaks forward edge kCFI since the underlying
> > function definition does not match the function hook definition.
> >
> > The return type of ieee80211_xmit should be changed from int to
> > netdev_tx_t.
> >
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> > Cc: llvm@lists.linux.dev
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
>
> The protoype in drivers/staging/rtl8192u/ieee80211/ieee80211.h should be
> updated as well. With that:

Nice catch, thanks.

>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> > ---
> >  drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c
> > index 8602e3a6c837..e4b6454809a0 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c
> > @@ -526,7 +526,7 @@ static void ieee80211_query_seqnum(struct ieee80211_device *ieee,
> >       }
> >  }
> >
> > -int ieee80211_xmit(struct sk_buff *skb, struct net_device *dev)
> > +netdev_tx_t ieee80211_xmit(struct sk_buff *skb, struct net_device *dev)
> >  {
> >       struct ieee80211_device *ieee = netdev_priv(dev);
> >       struct ieee80211_txb *txb = NULL;
> > @@ -822,13 +822,13 @@ int ieee80211_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       if ((*ieee->hard_start_xmit)(txb, dev) == 0) {
> >                               stats->tx_packets++;
> >                               stats->tx_bytes += __le16_to_cpu(txb->payload_size);
> > -                             return 0;
> > +                             return NETDEV_TX_OK;
> >                       }
> >                       ieee80211_txb_free(txb);
> >               }
> >       }
> >
> > -     return 0;
> > +     return NETDEV_TX_OK;
> >
> >   failed:
> >       spin_unlock_irqrestore(&ieee->lock, flags);
>
> I see 'return 1' down here, that doesn't appear to be a valid
> 'netdev_tx_t'. Should that be 'return NETDEV_TX_BUSY'? I guess that is
> an outstanding issue though, just figured I would mention it.

I'm not sure. As far as I can tell, the dev_xmit_complete function in
include/linux/netdevice.h is used to determine whether an skb was
consumed or not. It looks like a value of 1 (NET_XMIT_DROP) would
signify that it was consumed, whereas NETDEV_TX_BUSY would not.

It's somewhat confusing that a function which returns an enum may also
return values outside of that enum.

For more info see the foillowing commits:
dc1f8bf68b311b1537cb65893430b6796118498a
572a9d7b6fc7f20f573664063324c086be310c42
7f2e870f2a48a0524a3b03b04fa019311d16a7f7

>
> > --
> > 2.37.2.789.g6183377224-goog
> >
>
> Cheers,
> Nathan

Thanks,
Huck