linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit
@ 2022-09-12 21:45 Nathan Huckleberry
  2022-09-13 23:09 ` Nathan Chancellor
  0 siblings, 1 reply; 4+ messages in thread
From: Nathan Huckleberry @ 2022-09-12 21:45 UTC (permalink / raw)
  Cc: Nathan Huckleberry, Dan Carpenter, llvm, Greg Kroah-Hartman,
	Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-staging,
	linux-kernel

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>
---
 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);
-- 
2.37.2.789.g6183377224-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit
  2022-09-12 21:45 [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit Nathan Huckleberry
@ 2022-09-13 23:09 ` Nathan Chancellor
  2022-09-14  1:45   ` Nathan Huckleberry
  0 siblings, 1 reply; 4+ messages in thread
From: Nathan Chancellor @ 2022-09-13 23:09 UTC (permalink / raw)
  To: Nathan Huckleberry
  Cc: Dan Carpenter, llvm, Greg Kroah-Hartman, Nick Desaulniers,
	Tom Rix, linux-staging, linux-kernel

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:

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.

> -- 
> 2.37.2.789.g6183377224-goog
> 

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit
  2022-09-13 23:09 ` Nathan Chancellor
@ 2022-09-14  1:45   ` Nathan Huckleberry
  2022-09-14 21:07     ` [PATCH v2] " Nathan Huckleberry
  0 siblings, 1 reply; 4+ messages in thread
From: Nathan Huckleberry @ 2022-09-14  1:45 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Dan Carpenter, llvm, Greg Kroah-Hartman, Nick Desaulniers,
	Tom Rix, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] staging: rtl8192u: Fix return type of ieee80211_xmit
  2022-09-14  1:45   ` Nathan Huckleberry
@ 2022-09-14 21:07     ` Nathan Huckleberry
  0 siblings, 0 replies; 4+ messages in thread
From: Nathan Huckleberry @ 2022-09-14 21:07 UTC (permalink / raw)
  To: nhuck
  Cc: error27, gregkh, linux-kernel, linux-staging, llvm, nathan,
	ndesaulniers, trix

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>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Link: https://github.com/ClangBuiltLinux/linux/issues/1703
Cc: llvm@lists.linux.dev
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---

Changes v1 -> v2:
- Updated header file
- Added reviewed-by tag

 drivers/staging/rtl8192u/ieee80211/ieee80211.h    | 2 +-
 drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
index b577f9c81f85..9cd4b1896745 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
@@ -2178,7 +2178,7 @@ int ieee80211_set_encryption(struct ieee80211_device *ieee);
 int ieee80211_encrypt_fragment(struct ieee80211_device *ieee,
 			       struct sk_buff *frag, int hdr_len);
 
-int ieee80211_xmit(struct sk_buff *skb, struct net_device *dev);
+netdev_tx_t ieee80211_xmit(struct sk_buff *skb, struct net_device *dev);
 void ieee80211_txb_free(struct ieee80211_txb *txb);
 
 
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);
-- 
2.37.2.789.g6183377224-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-09-14 21:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-12 21:45 [PATCH] staging: rtl8192u: Fix return type of ieee80211_xmit Nathan Huckleberry
2022-09-13 23:09 ` Nathan Chancellor
2022-09-14  1:45   ` Nathan Huckleberry
2022-09-14 21:07     ` [PATCH v2] " Nathan Huckleberry

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).