[dpdk-dev,v3] net/i40e: fix multiple DDP packages should not be allowed
Checks
Commit Message
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(-)
Comments
> -----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>
> -----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
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.
> -----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.
> -----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.
> -----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
@@ -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;