[PATCH net-next] net: ixgbe: fix error handling in tc cls_u32 offload routines.

[PATCH net-next] net: ixgbe: fix error handling in tc cls_u32 offload routines.

[Sridhar Samudrala]

Check for handle ids when adding/deleting hash nodes OR adding/deleting
filter entries and limit them to max number of links or header nodes
supported(IXGBE_MAX_LINK_HANDLE).

Start from bit 0 when setting hash table bit-map.(adapter->tables)

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52 +++++++++++++++++----------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index b893ff8..10ccd96 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8209,10 +8209,17 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 static int ixgbe_delete_clsu32(struct ixgbe_adapter *adapter,
 			       struct tc_cls_u32_offload *cls)
 {
+	u32 uhtid = TC_U32_USERHTID(cls->knode.handle);
+	u32 loc;
 	int err;
 
+	if ((uhtid != 0x800) && (uhtid >= IXGBE_MAX_LINK_HANDLE))
+		return -EINVAL;
+
+	loc = cls->knode.handle & 0xfffff;
+
 	spin_lock(&adapter->fdir_perfect_lock);
-	err = ixgbe_update_ethtool_fdir_entry(adapter, NULL, cls->knode.handle);
+	err = ixgbe_update_ethtool_fdir_entry(adapter, NULL, loc);
 	spin_unlock(&adapter->fdir_perfect_lock);
 	return err;
 }
@@ -8221,20 +8228,30 @@ static int ixgbe_configure_clsu32_add_hnode(struct ixgbe_adapter *adapter,
 					    __be16 protocol,
 					    struct tc_cls_u32_offload *cls)
 {
+	u32 uhtid = TC_U32_USERHTID(cls->hnode.handle);
+
+	if (uhtid >= IXGBE_MAX_LINK_HANDLE)
+		return -EINVAL;
+
 	/* This ixgbe devices do not support hash tables at the moment
 	 * so abort when given hash tables.
 	 */
 	if (cls->hnode.divisor > 0)
 		return -EINVAL;
 
-	set_bit(TC_U32_USERHTID(cls->hnode.handle), &adapter->tables);
+	set_bit(uhtid-1, &adapter->tables);
 	return 0;
 }
 
 static int ixgbe_configure_clsu32_del_hnode(struct ixgbe_adapter *adapter,
 					    struct tc_cls_u32_offload *cls)
 {
-	clear_bit(TC_U32_USERHTID(cls->hnode.handle), &adapter->tables);
+	u32 uhtid = TC_U32_USERHTID(cls->hnode.handle);
+
+	if (uhtid >= IXGBE_MAX_LINK_HANDLE)
+		return -EINVAL;
+
+	clear_bit(uhtid-1, &adapter->tables);
 	return 0;
 }
 
@@ -8252,27 +8269,29 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 #endif
 	int i, err = 0;
 	u8 queue;
-	u32 handle;
+	u32 uhtid, link_uhtid;
 
 	memset(&mask, 0, sizeof(union ixgbe_atr_input));
-	handle = cls->knode.handle;
+	uhtid = TC_U32_USERHTID(cls->knode.handle);
+	link_uhtid = TC_U32_USERHTID(cls->knode.link_handle);
 
-	/* At the moment cls_u32 jumps to transport layer and skips past
+	/* At the moment cls_u32 jumps to network layer and skips past
 	 * L2 headers. The canonical method to match L2 frames is to use
 	 * negative values. However this is error prone at best but really
 	 * just broken because there is no way to "know" what sort of hdr
-	 * is in front of the transport layer. Fix cls_u32 to support L2
+	 * is in front of the network layer. Fix cls_u32 to support L2
 	 * headers when needed.
 	 */
 	if (protocol != htons(ETH_P_IP))
 		return -EINVAL;
 
-	if (cls->knode.link_handle ||
-	    cls->knode.link_handle >= IXGBE_MAX_LINK_HANDLE) {
+	if (link_uhtid) {
 		struct ixgbe_nexthdr *nexthdr = ixgbe_ipv4_jumps;
-		u32 uhtid = TC_U32_USERHTID(cls->knode.link_handle);
 
-		if (!test_bit(uhtid, &adapter->tables))
+		if (link_uhtid >= IXGBE_MAX_LINK_HANDLE)
+			return -EINVAL;
+
+		if (!test_bit(link_uhtid-1, &adapter->tables))
 			return -EINVAL;
 
 		for (i = 0; nexthdr[i].jump; i++) {
@@ -8288,10 +8307,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 			    nexthdr->mask != cls->knode.sel->keys[0].mask)
 				return -EINVAL;
 
-			if (uhtid >= IXGBE_MAX_LINK_HANDLE)
-				return -EINVAL;
-
-			adapter->jump_tables[uhtid] = nexthdr->jump;
+			adapter->jump_tables[link_uhtid] = nexthdr->jump;
 		}
 		return 0;
 	}
@@ -8308,13 +8324,13 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 	 * To add support for new nodes update ixgbe_model.h parse structures
 	 * this function _should_ be generic try not to hardcode values here.
 	 */
-	if (TC_U32_USERHTID(handle) == 0x800) {
+	if (uhtid == 0x800) {
 		field_ptr = adapter->jump_tables[0];
 	} else {
-		if (TC_U32_USERHTID(handle) >= ARRAY_SIZE(adapter->jump_tables))
+		if (uhtid >= IXGBE_MAX_LINK_HANDLE)
 			return -EINVAL;
 
-		field_ptr = adapter->jump_tables[TC_U32_USERHTID(handle)];
+		field_ptr = adapter->jump_tables[uhtid];
 	}
 
 	if (!field_ptr)
-- 
2.1.0

Re: [PATCH net-next] net: ixgbe: fix error handling in tc cls_u32 offload routines.

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Thu, 2016-03-03 at 15:19 -0800, Sridhar Samudrala wrote:
> Check for handle ids when adding/deleting hash nodes OR
> adding/deleting
> filter entries and limit them to max number of links or header nodes
> supported(IXGBE_MAX_LINK_HANDLE).
> 
> Start from bit 0 when setting hash table bit-map.(adapter->tables)
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52
> +++++++++++++++++----------
>  1 file changed, 34 insertions(+), 18 deletions(-)

Dave I will add this to my queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Intel-wired-lan] [net-next] ixgbe: fix error handling in tc cls_u32 offload routines

[Jeff Kirsher]

From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com>

Check for handle ids when adding/deleting hash nodes OR adding/deleting
filter entries and limit them to max number of links or header nodes
supported(IXGBE_MAX_LINK_HANDLE).

Start from bit 0 when setting hash table bit-map.(adapter->tables)

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---

Sending this to the correct mailing list (get_maintainer.pl is your
friend, please use it in the future)

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52 +++++++++++++++++----------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index b893ff8..10ccd96 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8209,10 +8209,17 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 static int ixgbe_delete_clsu32(struct ixgbe_adapter *adapter,
 			       struct tc_cls_u32_offload *cls)
 {
+	u32 uhtid = TC_U32_USERHTID(cls->knode.handle);
+	u32 loc;
 	int err;
 
+	if ((uhtid != 0x800) && (uhtid >= IXGBE_MAX_LINK_HANDLE))
+		return -EINVAL;
+
+	loc = cls->knode.handle & 0xfffff;
+
 	spin_lock(&adapter->fdir_perfect_lock);
-	err = ixgbe_update_ethtool_fdir_entry(adapter, NULL, cls->knode.handle);
+	err = ixgbe_update_ethtool_fdir_entry(adapter, NULL, loc);
 	spin_unlock(&adapter->fdir_perfect_lock);
 	return err;
 }
@@ -8221,20 +8228,30 @@ static int ixgbe_configure_clsu32_add_hnode(struct ixgbe_adapter *adapter,
 					    __be16 protocol,
 					    struct tc_cls_u32_offload *cls)
 {
+	u32 uhtid = TC_U32_USERHTID(cls->hnode.handle);
+
+	if (uhtid >= IXGBE_MAX_LINK_HANDLE)
+		return -EINVAL;
+
 	/* This ixgbe devices do not support hash tables at the moment
 	 * so abort when given hash tables.
 	 */
 	if (cls->hnode.divisor > 0)
 		return -EINVAL;
 
-	set_bit(TC_U32_USERHTID(cls->hnode.handle), &adapter->tables);
+	set_bit(uhtid-1, &adapter->tables);
 	return 0;
 }
 
 static int ixgbe_configure_clsu32_del_hnode(struct ixgbe_adapter *adapter,
 					    struct tc_cls_u32_offload *cls)
 {
-	clear_bit(TC_U32_USERHTID(cls->hnode.handle), &adapter->tables);
+	u32 uhtid = TC_U32_USERHTID(cls->hnode.handle);
+
+	if (uhtid >= IXGBE_MAX_LINK_HANDLE)
+		return -EINVAL;
+
+	clear_bit(uhtid-1, &adapter->tables);
 	return 0;
 }
 
@@ -8252,27 +8269,29 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 #endif
 	int i, err = 0;
 	u8 queue;
-	u32 handle;
+	u32 uhtid, link_uhtid;
 
 	memset(&mask, 0, sizeof(union ixgbe_atr_input));
-	handle = cls->knode.handle;
+	uhtid = TC_U32_USERHTID(cls->knode.handle);
+	link_uhtid = TC_U32_USERHTID(cls->knode.link_handle);
 
-	/* At the moment cls_u32 jumps to transport layer and skips past
+	/* At the moment cls_u32 jumps to network layer and skips past
 	 * L2 headers. The canonical method to match L2 frames is to use
 	 * negative values. However this is error prone at best but really
 	 * just broken because there is no way to "know" what sort of hdr
-	 * is in front of the transport layer. Fix cls_u32 to support L2
+	 * is in front of the network layer. Fix cls_u32 to support L2
 	 * headers when needed.
 	 */
 	if (protocol != htons(ETH_P_IP))
 		return -EINVAL;
 
-	if (cls->knode.link_handle ||
-	    cls->knode.link_handle >= IXGBE_MAX_LINK_HANDLE) {
+	if (link_uhtid) {
 		struct ixgbe_nexthdr *nexthdr = ixgbe_ipv4_jumps;
-		u32 uhtid = TC_U32_USERHTID(cls->knode.link_handle);
 
-		if (!test_bit(uhtid, &adapter->tables))
+		if (link_uhtid >= IXGBE_MAX_LINK_HANDLE)
+			return -EINVAL;
+
+		if (!test_bit(link_uhtid-1, &adapter->tables))
 			return -EINVAL;
 
 		for (i = 0; nexthdr[i].jump; i++) {
@@ -8288,10 +8307,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 			    nexthdr->mask != cls->knode.sel->keys[0].mask)
 				return -EINVAL;
 
-			if (uhtid >= IXGBE_MAX_LINK_HANDLE)
-				return -EINVAL;
-
-			adapter->jump_tables[uhtid] = nexthdr->jump;
+			adapter->jump_tables[link_uhtid] = nexthdr->jump;
 		}
 		return 0;
 	}
@@ -8308,13 +8324,13 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 	 * To add support for new nodes update ixgbe_model.h parse structures
 	 * this function _should_ be generic try not to hardcode values here.
 	 */
-	if (TC_U32_USERHTID(handle) == 0x800) {
+	if (uhtid == 0x800) {
 		field_ptr = adapter->jump_tables[0];
 	} else {
-		if (TC_U32_USERHTID(handle) >= ARRAY_SIZE(adapter->jump_tables))
+		if (uhtid >= IXGBE_MAX_LINK_HANDLE)
 			return -EINVAL;
 
-		field_ptr = adapter->jump_tables[TC_U32_USERHTID(handle)];
+		field_ptr = adapter->jump_tables[uhtid];
 	}
 
 	if (!field_ptr)

[Intel-wired-lan] [net-next] ixgbe: fix error handling in tc cls_u32 offload routines

[John Fastabend]

On 16-03-03 03:42 PM, Jeff Kirsher wrote:
> From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com>
> 
> Check for handle ids when adding/deleting hash nodes OR adding/deleting
> filter entries and limit them to max number of links or header nodes
> supported(IXGBE_MAX_LINK_HANDLE).
> 
> Start from bit 0 when setting hash table bit-map.(adapter->tables)
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
> 
> Sending this to the correct mailing list (get_maintainer.pl is your
> friend, please use it in the future)
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52 +++++++++++++++++----------
>  1 file changed, 34 insertions(+), 18 deletions(-)
> 


Looks like a good cleanup to me.

Acked-by: John Fastabend <john.r.fastabend@intel.com>

[Intel-wired-lan] [net-next] ixgbe: fix error handling in tc cls_u32 offload routines

[John Fastabend]

On 16-03-03 04:29 PM, John Fastabend wrote:
> On 16-03-03 03:42 PM, Jeff Kirsher wrote:
>> From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com>
>>
>> Check for handle ids when adding/deleting hash nodes OR adding/deleting
>> filter entries and limit them to max number of links or header nodes
>> supported(IXGBE_MAX_LINK_HANDLE).
>>
>> Start from bit 0 when setting hash table bit-map.(adapter->tables)
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>>
>> Sending this to the correct mailing list (get_maintainer.pl is your
>> friend, please use it in the future)
>>
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52 +++++++++++++++++----------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
>>
> 
> 
> Looks like a good cleanup to me.
> 
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
> 

Also just checking Sridhar you tested both the adding just to
the first list e.g. matching ip address for example and doing
matches deeper in the stack ala match on sport/dport?

Thanks,
John

[Intel-wired-lan] [net-next] ixgbe: fix error handling in tc cls_u32 offload routines

[Samudrala, Sridhar]

On 3/3/2016 4:30 PM, John Fastabend wrote:
> On 16-03-03 04:29 PM, John Fastabend wrote:
>> On 16-03-03 03:42 PM, Jeff Kirsher wrote:
>>> From: "Samudrala, Sridhar" <sridhar.samudrala@intel.com>
>>>
>>> Check for handle ids when adding/deleting hash nodes OR adding/deleting
>>> filter entries and limit them to max number of links or header nodes
>>> supported(IXGBE_MAX_LINK_HANDLE).
>>>
>>> Start from bit 0 when setting hash table bit-map.(adapter->tables)
>>>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> ---
>>>
>>> Sending this to the correct mailing list (get_maintainer.pl is your
>>> friend, please use it in the future)
>>>
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52 +++++++++++++++++----------
>>>   1 file changed, 34 insertions(+), 18 deletions(-)
>>>
>>
>> Looks like a good cleanup to me.
>>
>> Acked-by: John Fastabend <john.r.fastabend@intel.com>
>>
> Also just checking Sridhar you tested both the adding just to
> the first list e.g. matching ip address for example and doing
> matches deeper in the stack ala match on sport/dport?

Yes.

I tested adding matches on src/dst ip on the root hash table and matches 
on tcp src port on the link hash table.

tc filter add dev p4p1 parent ffff: protocol ip prio 99 handle 800:0:1 
u32 ht 800: match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop

This creates the following flow director rule as seen via ethtool

Filter: 1
     Rule Type: Raw IPv4
     Src IP addr: 15.0.0.2 mask: 0.0.0.0
     Dest IP addr: 15.0.0.1 mask: 0.0.0.0
     TOS: 0x0 mask: 0xff
     Protocol: 0 mask: 0xff
     L4 bytes: 0x0 mask: 0xffffffff
     VLAN EtherType: 0x0 mask: 0xffff
     VLAN: 0x0 mask: 0xffff
     User-defined: 0x0 mask: 0xffffffffffffffff
     Action: Drop

To do tcp matches,  i had to delete ip match rule in the root hash 
table,  create a new hash table linked to the root and add a filter 
entry to that table.

c filter del dev p4p1 parent ffff: protocol ip prio 99 handle 800:0:1 u32
tc filter add dev p4p1 parent ffff: protocol ip prio 99 handle 1: u32 
divisor 1
tc filter add dev p4p1 protocol ip parent ffff: prio 99 u32 ht 800: 
order 2 link 1: offset at 0 mask 0f00 shift 6 plus 0 eat match ip 
protocol 6 ff
tc filter add dev p4p1 parent ffff: protocol ip prio 99 u32 ht 1: order 
3 match tcp src 23 ffff action drop

This creates the following  flow director rule
Filter: 3
     Rule Type: TCP over IPv4
     Src IP addr: 0.0.0.0 mask: 255.255.255.255
     Dest IP addr: 0.0.0.0 mask: 255.255.255.255
     TOS: 0x0 mask: 0xff
     Src port: 23 mask: 0x0
     Dest port: 0 mask: 0xffff
     VLAN EtherType: 0x0 mask: 0xffff
     VLAN: 0x0 mask: 0xffff
     User-defined: 0x0 mask: 0xffffffffffffffff
     Action: Drop

Looks like we cannot have both these rules at the same time.

Also, more work is required to support matches on both ip as well as tcp 
headers in the same rule.

Thanks
Sridhar

Re: [PATCH net-next] net: ixgbe: fix error handling in tc cls_u32 offload routines.

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 03 Mar 2016 15:37:52 -0800

> On Thu, 2016-03-03 at 15:19 -0800, Sridhar Samudrala wrote:
>> Check for handle ids when adding/deleting hash nodes OR
>> adding/deleting
>> filter entries and limit them to max number of links or header nodes
>> supported(IXGBE_MAX_LINK_HANDLE).
>> 
>> Start from bit 0 when setting hash table bit-map.(adapter->tables)
>> 
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 52
>> +++++++++++++++++----------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> Dave I will add this to my queue

Ok.