[PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[Felix Fietkau]

Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
flow_block_cb_incref unconditionally, even for a newly allocated cb.
Fixes a use-after-free bug. Also fix the accidentally inverted refcount
check on unbind.

Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 .../net/ethernet/mediatek/mtk_ppe_offload.c   | 23 +++++++++++--------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 81afd5ee3fbf..43cc510b51bd 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
 	struct mtk_eth *eth = mac->hw;
 	static LIST_HEAD(block_cb_list);
 	struct flow_block_cb *block_cb;
+	bool register_block = false;
 	flow_setup_cb_t *cb;
 
 	if (!eth->soc->offload_version)
@@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
 	switch (f->command) {
 	case FLOW_BLOCK_BIND:
 		block_cb = flow_block_cb_lookup(f->block, cb, dev);
-		if (block_cb) {
-			flow_block_cb_incref(block_cb);
-			return 0;
+		if (!block_cb) {
+			block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
+			if (IS_ERR(block_cb))
+				return PTR_ERR(block_cb);
+
+			register_block = true;
 		}
-		block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
-		if (IS_ERR(block_cb))
-			return PTR_ERR(block_cb);
 
-		flow_block_cb_add(block_cb, f);
-		list_add_tail(&block_cb->driver_list, &block_cb_list);
+		flow_block_cb_incref(block_cb);
+
+		if (register_block) {
+			flow_block_cb_add(block_cb, f);
+			list_add_tail(&block_cb->driver_list, &block_cb_list);
+		}
 		return 0;
 	case FLOW_BLOCK_UNBIND:
 		block_cb = flow_block_cb_lookup(f->block, cb, dev);
 		if (!block_cb)
 			return -ENOENT;
 
-		if (flow_block_cb_decref(block_cb)) {
+		if (!flow_block_cb_decref(block_cb)) {
 			flow_block_cb_remove(block_cb, f);
 			list_del(&block_cb->driver_list);
 		}
-- 
2.39.0

[PATCH net 2/2] net: ethernet: mtk_eth_soc: fix L2 offloading with DSA untag offload enabled

[Felix Fietkau]

Check for skb metadata in order to detect the case where the DSA header is not
present.

Fixes: 2d7605a72906 ("net: ethernet: mtk_eth_soc: enable hardware DSA untagging")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 6 +++---
 drivers/net/ethernet/mediatek/mtk_ppe.c     | 5 ++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 3cb43623d3db..a94aa08515af 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2059,9 +2059,6 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 			skb_checksum_none_assert(skb);
 		skb->protocol = eth_type_trans(skb, netdev);
 
-		if (reason == MTK_PPE_CPU_REASON_HIT_UNBIND_RATE_REACHED)
-			mtk_ppe_check_skb(eth->ppe[0], skb, hash);
-
 		if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX) {
 			if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
 				if (trxd.rxd3 & RX_DMA_VTAG_V2) {
@@ -2089,6 +2086,9 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 			__vlan_hwaccel_put_tag(skb, htons(vlan_proto), vlan_tci);
 		}
 
+		if (reason == MTK_PPE_CPU_REASON_HIT_UNBIND_RATE_REACHED)
+			mtk_ppe_check_skb(eth->ppe[0], skb, hash);
+
 		skb_record_rx_queue(skb, 0);
 		napi_gro_receive(napi, skb);
 
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 6883eb34cd8b..a038b99ecbda 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -8,6 +8,7 @@
 #include <linux/platform_device.h>
 #include <linux/if_ether.h>
 #include <linux/if_vlan.h>
+#include <net/dst_metadata.h>
 #include <net/dsa.h>
 #include "mtk_eth_soc.h"
 #include "mtk_ppe.h"
@@ -699,7 +700,9 @@ void __mtk_ppe_check_skb(struct mtk_ppe *ppe, struct sk_buff *skb, u16 hash)
 		    skb->dev->dsa_ptr->tag_ops->proto != DSA_TAG_PROTO_MTK)
 			goto out;
 
-		tag += 4;
+		if (!skb_metadata_dst(skb))
+			tag += 4;
+
 		if (get_unaligned_be16(tag) != ETH_P_8021Q)
 			break;
 
-- 
2.39.0

Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[Simon Horman]

On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote:
> Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
> flow_block_cb_incref unconditionally, even for a newly allocated cb.
> Fixes a use-after-free bug. Also fix the accidentally inverted refcount
> check on unbind.

Hi Felix,

my 2c worth.

Firstly, it's usually best to have one fix per patch.

Second, this seems to conflicts with:

  [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for
        offloading flows from wlan devices

So I guess that series will need to be rebased on this one at some point.

> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  .../net/ethernet/mediatek/mtk_ppe_offload.c   | 23 +++++++++++--------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 81afd5ee3fbf..43cc510b51bd 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	struct mtk_eth *eth = mac->hw;
>  	static LIST_HEAD(block_cb_list);
>  	struct flow_block_cb *block_cb;
> +	bool register_block = false;
>  	flow_setup_cb_t *cb;
>  
>  	if (!eth->soc->offload_version)
> @@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	switch (f->command) {
>  	case FLOW_BLOCK_BIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
> -		if (block_cb) {
> -			flow_block_cb_incref(block_cb);
> -			return 0;
> +		if (!block_cb) {
> +			block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> +			if (IS_ERR(block_cb))
> +				return PTR_ERR(block_cb);
> +
> +			register_block = true;
>  		}
> -		block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> -		if (IS_ERR(block_cb))
> -			return PTR_ERR(block_cb);
>  
> -		flow_block_cb_add(block_cb, f);
> -		list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		flow_block_cb_incref(block_cb);
> +
> +		if (register_block) {
> +			flow_block_cb_add(block_cb, f);
> +			list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		}
>  		return 0;

I think the existing code was more idiomatic, and could
be maintained by simply adding a call to flow_block_cb_incref()
before the call to flow_block_cb_add().

@@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
 		if (IS_ERR(block_cb))
 			return PTR_ERR(block_cb);
 
+		flow_block_cb_incref(block_cb);
 		flow_block_cb_add(block_cb, f);
 		list_add_tail(&block_cb->driver_list, &block_cb_list);
 		return 0;

>  	case FLOW_BLOCK_UNBIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
>  		if (!block_cb)
>  			return -ENOENT;
>  
> -		if (flow_block_cb_decref(block_cb)) {
> +		if (!flow_block_cb_decref(block_cb)) {
>  			flow_block_cb_remove(block_cb, f);
>  			list_del(&block_cb->driver_list);
>  		}
> -- 
> 2.39.0
> 

Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[Jakub Kicinski]

On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote:
> On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote:
> > Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
> > flow_block_cb_incref unconditionally, even for a newly allocated cb.
> > Fixes a use-after-free bug. Also fix the accidentally inverted refcount
> > check on unbind.  
> 
> Firstly, it's usually best to have one fix per patch.

Or at least reword the commit message to make it look like it's fixing
the refcounting logic?

> >  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
> > -		if (block_cb) {
> > -			flow_block_cb_incref(block_cb);
> > -			return 0;
> > +		if (!block_cb) {
> > +			block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> > +			if (IS_ERR(block_cb))
> > +				return PTR_ERR(block_cb);
> > +
> > +			register_block = true;
> >  		}
> > -		block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> > -		if (IS_ERR(block_cb))
> > -			return PTR_ERR(block_cb);
> >  
> > -		flow_block_cb_add(block_cb, f);
> > -		list_add_tail(&block_cb->driver_list, &block_cb_list);
> > +		flow_block_cb_incref(block_cb);
> > +
> > +		if (register_block) {
> > +			flow_block_cb_add(block_cb, f);
> > +			list_add_tail(&block_cb->driver_list, &block_cb_list);
> > +		}
> >  		return 0;  
> 
> I think the existing code was more idiomatic, and could
> be maintained by simply adding a call to flow_block_cb_incref()
> before the call to flow_block_cb_add().
> 
> @@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  		if (IS_ERR(block_cb))
>  			return PTR_ERR(block_cb);
>  
> +		flow_block_cb_incref(block_cb);
>  		flow_block_cb_add(block_cb, f);
>  		list_add_tail(&block_cb->driver_list, &block_cb_list);
>  		return 0;

That'd be my go to fix as well, FWIW.

Alternatively - the block cannot be removed until FLOW_BLOCK_UNBIND is
called, right? So the register_block bool can be skipped and
refcount taken after flow_block_cb_add() / list_add_tail().
That way at least the new block handling doesn't have to be split
into two chunks.

Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting

[Felix Fietkau]

On 23.03.23 06:37, Jakub Kicinski wrote:
> On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote:
>> On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote:
>> > Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
>> > flow_block_cb_incref unconditionally, even for a newly allocated cb.
>> > Fixes a use-after-free bug. Also fix the accidentally inverted refcount
>> > check on unbind.  
>> 
>> Firstly, it's usually best to have one fix per patch.
> 
> Or at least reword the commit message to make it look like it's fixing
> the refcounting logic?
I thought that was clear in the title already :)
I've considered splitting the patch in two, but decided against it, 
because it could cause bisect issues.
Right now the accidentally inverted logic is preventing the 
use-after-free bug from showing up with a single flow block, so that 
would break if I only fix one part without the other.

Will send v2 based on your suggestions.

- Felix