[dpdk-dev] [PATCH v2] net/i40e: fix multiple DDP packages should not be allowed

[dpdk-dev] [PATCH v2] net/i40e: fix multiple DDP packages should not be allowed

[Kirill Rybalchenko]

Should be not possible to load conflicting DDP profiles.
Only DDP profiles of the same group (not 0) can be loaded
together;
If DDP profile group is 0, it is exclusive, i.e. it cannot
be loaded with any other DDP profile;
If DDP profile groups are different - these profiles cannot
be loaded together;

Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP processing")

Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
---
 drivers/net/i40e/rte_pmd_i40e.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
index 5436db4..be37912 100644
--- a/drivers/net/i40e/rte_pmd_i40e.c
+++ b/drivers/net/i40e/rte_pmd_i40e.c
@@ -1496,6 +1496,7 @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
 	struct rte_pmd_i40e_profile_info *pinfo, *p;
 	uint32_t i;
 	int ret;
+	static const int pgroup = 0x00ff0000;
 
 	buff = rte_zmalloc("pinfo_list",
 			   (I40E_PROFILE_INFO_SIZE * I40E_MAX_PROFILE_NUM + 4),
@@ -1525,6 +1526,22 @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
 			return 1;
 		}
 	}
+	for (i = 0; i < p_list->p_count; i++) {
+		p = &p_list->p_info[i];
+		if ((p->track_id & pgroup) == 0) {
+			PMD_DRV_LOG(INFO, "Profile of the grpoup 0 exists.");
+			rte_free(buff);
+			return 2;
+		}
+	}
+	for (i = 0; i < p_list->p_count; i++) {
+		p = &p_list->p_info[i];
+		if ((pinfo->track_id & pgroup) != (p->track_id & pgroup)) {
+			PMD_DRV_LOG(INFO, "Profile of different group exists.");
+			rte_free(buff);
+			return 3;
+		}
+	}
 
 	rte_free(buff);
 	return 0;
@@ -1628,12 +1645,17 @@ rte_pmd_i40e_process_ddp_package(uint16_t port, uint8_t *buff,
 
 	if (op == RTE_PMD_I40E_PKG_OP_WR_ADD) {
 		if (is_exist) {
-			PMD_DRV_LOG(ERR, "Profile already exists.");
+			if (is_exist == 1)
+				PMD_DRV_LOG(ERR, "Profile already exists.");
+			else if (is_exist == 2)
+				PMD_DRV_LOG(ERR, "Profile of group 0 already exists.");
+			else if (is_exist == 3)
+				PMD_DRV_LOG(ERR, "Profile of different group already exists");
 			rte_free(profile_info_sec);
 			return -EEXIST;
 		}
 	} else if (op == RTE_PMD_I40E_PKG_OP_WR_DEL) {
-		if (!is_exist) {
+		if (is_exist != 1) {
 			PMD_DRV_LOG(ERR, "Profile does not exist.");
 			rte_free(profile_info_sec);
 			return -EACCES;
-- 
2.5.5

[dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Kirill Rybalchenko]

Should be not possible to load conflicting DDP profiles.
Only DDP profiles of the same group (not 0) can be loaded
together;
If DDP profile group is 0, it is exclusive, i.e. it cannot
be loaded with any other DDP profile;
If DDP profile groups are different - these profiles cannot
be loaded together;

Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP processing")

v3: prevent registration of read-only profiles with profile list

Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
---
 drivers/net/i40e/rte_pmd_i40e.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
index 5436db4..dae59e6 100644
--- a/drivers/net/i40e/rte_pmd_i40e.c
+++ b/drivers/net/i40e/rte_pmd_i40e.c
@@ -1496,7 +1496,14 @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
 	struct rte_pmd_i40e_profile_info *pinfo, *p;
 	uint32_t i;
 	int ret;
+	static const uint32_t group_mask = 0x00ff0000;
 
+	pinfo = (struct rte_pmd_i40e_profile_info *)(profile_info_sec +
+			     sizeof(struct i40e_profile_section_header));
+	if (pinfo->track_id == 0) {
+		PMD_DRV_LOG(INFO, "Read-only profile.");
+		return 0;
+	}
 	buff = rte_zmalloc("pinfo_list",
 			   (I40E_PROFILE_INFO_SIZE * I40E_MAX_PROFILE_NUM + 4),
 			   0);
@@ -1515,8 +1522,6 @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
 		return -1;
 	}
 	p_list = (struct rte_pmd_i40e_profile_list *)buff;
-	pinfo = (struct rte_pmd_i40e_profile_info *)(profile_info_sec +
-			     sizeof(struct i40e_profile_section_header));
 	for (i = 0; i < p_list->p_count; i++) {
 		p = &p_list->p_info[i];
 		if (pinfo->track_id == p->track_id) {
@@ -1525,6 +1530,23 @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
 			return 1;
 		}
 	}
+	for (i = 0; i < p_list->p_count; i++) {
+		p = &p_list->p_info[i];
+		if ((p->track_id & group_mask) == 0) {
+			PMD_DRV_LOG(INFO, "Profile of the group 0 exists.");
+			rte_free(buff);
+			return 2;
+		}
+	}
+	for (i = 0; i < p_list->p_count; i++) {
+		p = &p_list->p_info[i];
+		if ((pinfo->track_id & group_mask) !=
+		    (p->track_id & group_mask)) {
+			PMD_DRV_LOG(INFO, "Profile of different group exists.");
+			rte_free(buff);
+			return 3;
+		}
+	}
 
 	rte_free(buff);
 	return 0;
@@ -1544,6 +1566,7 @@ rte_pmd_i40e_process_ddp_package(uint16_t port, uint8_t *buff,
 	uint8_t *profile_info_sec;
 	int is_exist;
 	enum i40e_status_code status = I40E_SUCCESS;
+	static const uint32_t type_mask = 0xff000000;
 
 	if (op != RTE_PMD_I40E_PKG_OP_WR_ADD &&
 		op != RTE_PMD_I40E_PKG_OP_WR_ONLY &&
@@ -1595,6 +1618,10 @@ rte_pmd_i40e_process_ddp_package(uint16_t port, uint8_t *buff,
 		return -EINVAL;
 	}
 
+	/* force read-only track_id for type 0 */
+	if ((track_id & type_mask) == 0)
+		track_id = 0;
+
 	/* Find profile segment */
 	profile_seg_hdr = i40e_find_segment_in_package(SEGMENT_TYPE_I40E,
 						       pkg_hdr);
@@ -1628,12 +1655,17 @@ rte_pmd_i40e_process_ddp_package(uint16_t port, uint8_t *buff,
 
 	if (op == RTE_PMD_I40E_PKG_OP_WR_ADD) {
 		if (is_exist) {
-			PMD_DRV_LOG(ERR, "Profile already exists.");
+			if (is_exist == 1)
+				PMD_DRV_LOG(ERR, "Profile already exists.");
+			else if (is_exist == 2)
+				PMD_DRV_LOG(ERR, "Profile of group 0 already exists.");
+			else if (is_exist == 3)
+				PMD_DRV_LOG(ERR, "Profile of different group already exists");
 			rte_free(profile_info_sec);
 			return -EEXIST;
 		}
 	} else if (op == RTE_PMD_I40E_PKG_OP_WR_DEL) {
-		if (!is_exist) {
+		if (is_exist != 1) {
 			PMD_DRV_LOG(ERR, "Profile does not exist.");
 			rte_free(profile_info_sec);
 			return -EACCES;
-- 
2.5.5

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Chilikin, Andrey]

> -----Original Message-----
> From: Rybalchenko, Kirill
> Sent: Thursday, February 1, 2018 12:43 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Rybalchenko, Kirill <kirill.rybalchenko@intel.com>;
> Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> allowed
> 
> Should be not possible to load conflicting DDP profiles.
> Only DDP profiles of the same group (not 0) can be loaded
> together;
> If DDP profile group is 0, it is exclusive, i.e. it cannot
> be loaded with any other DDP profile;
> If DDP profile groups are different - these profiles cannot
> be loaded together;
> 
> Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP
> processing")
> 
> v3: prevent registration of read-only profiles with profile list
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
Acked-by: Andrey Chilikin <andrey.chilikin@intel.com>

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Xing, Beilei]

> -----Original Message-----
> From: Rybalchenko, Kirill
> Sent: Thursday, February 1, 2018 8:43 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Rybalchenko, Kirill <kirill.rybalchenko@intel.com>;
> Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> allowed
> 
> Should be not possible to load conflicting DDP profiles.
> Only DDP profiles of the same group (not 0) can be loaded together; If DDP
> profile group is 0, it is exclusive, i.e. it cannot be loaded with any other DDP
> profile; If DDP profile groups are different - these profiles cannot be loaded
> together;
> 
> Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP
> processing")
> 
> v3: prevent registration of read-only profiles with profile list
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> ---
>  drivers/net/i40e/rte_pmd_i40e.c | 40
> ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/i40e/rte_pmd_i40e.c
> b/drivers/net/i40e/rte_pmd_i40e.c index 5436db4..dae59e6 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.c
> +++ b/drivers/net/i40e/rte_pmd_i40e.c
> @@ -1496,7 +1496,14 @@ i40e_check_profile_info(uint16_t port, uint8_t
> *profile_info_sec)
>  	struct rte_pmd_i40e_profile_info *pinfo, *p;
>  	uint32_t i;
>  	int ret;
> +	static const uint32_t group_mask = 0x00ff0000;
> 
> +	pinfo = (struct rte_pmd_i40e_profile_info *)(profile_info_sec +
> +			     sizeof(struct i40e_profile_section_header));
> +	if (pinfo->track_id == 0) {
> +		PMD_DRV_LOG(INFO, "Read-only profile.");
> +		return 0;
> +	}
>  	buff = rte_zmalloc("pinfo_list",
>  			   (I40E_PROFILE_INFO_SIZE *
> I40E_MAX_PROFILE_NUM + 4),
>  			   0);
> @@ -1515,8 +1522,6 @@ i40e_check_profile_info(uint16_t port, uint8_t
> *profile_info_sec)
>  		return -1;
>  	}
>  	p_list = (struct rte_pmd_i40e_profile_list *)buff;
> -	pinfo = (struct rte_pmd_i40e_profile_info *)(profile_info_sec +
> -			     sizeof(struct i40e_profile_section_header));
>  	for (i = 0; i < p_list->p_count; i++) {
>  		p = &p_list->p_info[i];
>  		if (pinfo->track_id == p->track_id) { @@ -1525,6 +1530,23
> @@ i40e_check_profile_info(uint16_t port, uint8_t *profile_info_sec)
>  			return 1;
>  		}
>  	}
> +	for (i = 0; i < p_list->p_count; i++) {
> +		p = &p_list->p_info[i];
> +		if ((p->track_id & group_mask) == 0) {
> +			PMD_DRV_LOG(INFO, "Profile of the group 0
> exists.");
> +			rte_free(buff);
> +			return 2;
> +		}
> +	}
> +	for (i = 0; i < p_list->p_count; i++) {
> +		p = &p_list->p_info[i];
> +		if ((pinfo->track_id & group_mask) !=
> +		    (p->track_id & group_mask)) {
> +			PMD_DRV_LOG(INFO, "Profile of different group
> exists.");
> +			rte_free(buff);
> +			return 3;
> +		}
> +	}
> 
>  	rte_free(buff);
>  	return 0;
> @@ -1544,6 +1566,7 @@ rte_pmd_i40e_process_ddp_package(uint16_t
> port, uint8_t *buff,
>  	uint8_t *profile_info_sec;
>  	int is_exist;
>  	enum i40e_status_code status = I40E_SUCCESS;
> +	static const uint32_t type_mask = 0xff000000;
> 
>  	if (op != RTE_PMD_I40E_PKG_OP_WR_ADD &&
>  		op != RTE_PMD_I40E_PKG_OP_WR_ONLY &&
> @@ -1595,6 +1618,10 @@ rte_pmd_i40e_process_ddp_package(uint16_t
> port, uint8_t *buff,
>  		return -EINVAL;
>  	}
> 
> +	/* force read-only track_id for type 0 */
> +	if ((track_id & type_mask) == 0)
> +		track_id = 0;
> +
>  	/* Find profile segment */
>  	profile_seg_hdr =
> i40e_find_segment_in_package(SEGMENT_TYPE_I40E,
>  						       pkg_hdr);
> @@ -1628,12 +1655,17 @@ rte_pmd_i40e_process_ddp_package(uint16_t
> port, uint8_t *buff,
> 
>  	if (op == RTE_PMD_I40E_PKG_OP_WR_ADD) {
>  		if (is_exist) {

How about removing the above if statement since there're 3 if statements for is_exist below?

> -			PMD_DRV_LOG(ERR, "Profile already exists.");
> +			if (is_exist == 1)
> +				PMD_DRV_LOG(ERR, "Profile already
> exists.");
> +			else if (is_exist == 2)
> +				PMD_DRV_LOG(ERR, "Profile of group 0
> already exists.");
> +			else if (is_exist == 3)
> +				PMD_DRV_LOG(ERR, "Profile of different
> group already exists");
>  			rte_free(profile_info_sec);
>  			return -EEXIST;
>  		}
>  	} else if (op == RTE_PMD_I40E_PKG_OP_WR_DEL) {
> -		if (!is_exist) {
> +		if (is_exist != 1) {
>  			PMD_DRV_LOG(ERR, "Profile does not exist.");
>  			rte_free(profile_info_sec);
>  			return -EACCES;
> --
> 2.5.5

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Rybalchenko, Kirill]

Hi Beilei,

> -----Original Message-----
> From: Xing, Beilei
> Sent: Monday 5 February 2018 10:41
> To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Subject: RE: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> allowed
> 
> > @@ -1628,12 +1655,17 @@ rte_pmd_i40e_process_ddp_package(uint16_t
> > port, uint8_t *buff,
> >
> >  	if (op == RTE_PMD_I40E_PKG_OP_WR_ADD) {
> >  		if (is_exist) {
> 
> How about removing the above if statement since there're 3 if statements
> for is_exist below?


This if statement is necessary because these two lines
			rte_free(profile_info_sec);
  			return -EEXIST;
should be executed only if is_exist has non-zero value.
Statements
			if (is_exist == 1, 2, 3)
are only selector for appropriate log message.

Or did I misunderstand your idea? 

> 
> > -			PMD_DRV_LOG(ERR, "Profile already exists.");
> > +			if (is_exist == 1)
> > +				PMD_DRV_LOG(ERR, "Profile already
> > exists.");
> > +			else if (is_exist == 2)
> > +				PMD_DRV_LOG(ERR, "Profile of group 0
> > already exists.");
> > +			else if (is_exist == 3)
> > +				PMD_DRV_LOG(ERR, "Profile of different
> > group already exists");
> >  			rte_free(profile_info_sec);
> >  			return -EEXIST;
> >  		}
> >  	} else if (op == RTE_PMD_I40E_PKG_OP_WR_DEL) {
> > -		if (!is_exist) {
> > +		if (is_exist != 1) {
> >  			PMD_DRV_LOG(ERR, "Profile does not exist.");
> >  			rte_free(profile_info_sec);
> >  			return -EACCES;
> > --
> > 2.5.5

Regards,
Kirill.

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Xing, Beilei]

> -----Original Message-----
> From: Rybalchenko, Kirill
> Sent: Monday, February 5, 2018 9:59 PM
> To: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Subject: RE: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> allowed
> 
> Hi Beilei,
> 
> > -----Original Message-----
> > From: Xing, Beilei
> > Sent: Monday 5 February 2018 10:41
> > To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>
> > Subject: RE: [PATCH v3] net/i40e: fix multiple DDP packages should not
> > be allowed
> >
> > > @@ -1628,12 +1655,17 @@
> rte_pmd_i40e_process_ddp_package(uint16_t
> > > port, uint8_t *buff,
> > >
> > >  	if (op == RTE_PMD_I40E_PKG_OP_WR_ADD) {
> > >  		if (is_exist) {
> >
> > How about removing the above if statement since there're 3 if
> > statements for is_exist below?
> 
> 
> This if statement is necessary because these two lines
> 			rte_free(profile_info_sec);
>   			return -EEXIST;
> should be executed only if is_exist has non-zero value.
> Statements
> 			if (is_exist == 1, 2, 3)
> are only selector for appropriate log message.
> 
> Or did I misunderstand your idea?

Sorry I didn't comment clearly, actually I just think if we can refine the code here to less indentations.
However, the code overall looks OK for me. Will ACK it later. Thanks.

> 
> >
> > > -			PMD_DRV_LOG(ERR, "Profile already exists.");
> > > +			if (is_exist == 1)
> > > +				PMD_DRV_LOG(ERR, "Profile already
> > > exists.");
> > > +			else if (is_exist == 2)
> > > +				PMD_DRV_LOG(ERR, "Profile of group 0
> > > already exists.");
> > > +			else if (is_exist == 3)
> > > +				PMD_DRV_LOG(ERR, "Profile of different
> > > group already exists");
> > >  			rte_free(profile_info_sec);
> > >  			return -EEXIST;
> > >  		}
> > >  	} else if (op == RTE_PMD_I40E_PKG_OP_WR_DEL) {
> > > -		if (!is_exist) {
> > > +		if (is_exist != 1) {
> > >  			PMD_DRV_LOG(ERR, "Profile does not exist.");
> > >  			rte_free(profile_info_sec);
> > >  			return -EACCES;
> > > --
> > > 2.5.5
> 
> Regards,
> Kirill.

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Xing, Beilei]

> -----Original Message-----
> From: Rybalchenko, Kirill
> Sent: Thursday, February 1, 2018 8:43 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Rybalchenko, Kirill <kirill.rybalchenko@intel.com>;
> Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> allowed
> 
> Should be not possible to load conflicting DDP profiles.
> Only DDP profiles of the same group (not 0) can be loaded together; If DDP
> profile group is 0, it is exclusive, i.e. it cannot be loaded with any other DDP
> profile; If DDP profile groups are different - these profiles cannot be loaded
> together;
> 
> Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP
> processing")
> 
> v3: prevent registration of read-only profiles with profile list
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>

Acked-by: Beilei Xing <beilei.xing@intel.com>, thanks.

Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should not be allowed

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> Sent: Tuesday, February 6, 2018 10:27 AM
> To: Rybalchenko, Kirill; dev@dpdk.org
> Cc: stable@dpdk.org; Chilikin, Andrey; Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: fix multiple DDP packages should
> not be allowed
> 
> 
> 
> > -----Original Message-----
> > From: Rybalchenko, Kirill
> > Sent: Thursday, February 1, 2018 8:43 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Rybalchenko, Kirill
> > <kirill.rybalchenko@intel.com>; Chilikin, Andrey
> > <andrey.chilikin@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>
> > Subject: [PATCH v3] net/i40e: fix multiple DDP packages should not be
> > allowed
> >
> > Should be not possible to load conflicting DDP profiles.
> > Only DDP profiles of the same group (not 0) can be loaded together; If
> > DDP profile group is 0, it is exclusive, i.e. it cannot be loaded with
> > any other DDP profile; If DDP profile groups are different - these
> > profiles cannot be loaded together;
> >
> > Fixes: b319712f53c8 ("net/i40e: extended list of operations for DDP
> > processing")
> >
> > v3: prevent registration of read-only profiles with profile list
> >
> > Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> 
> Acked-by: Beilei Xing <beilei.xing@intel.com>, thanks.
Applied to dpdk-next-net-intel, with commit log changes. Thanks!

/Helin