| Message ID | 20210118085937.12072-1-alvinx.zhang@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | app/testpmd: fix RSS key | expand |
| Context | Check | Description |
|---|---|---|
| ci/iol-testing | warning | Testing issues |
| ci/iol-abi-testing | success | Testing PASS |
| ci/iol-testing | warning | Testing issues |
| ci/iol-abi-testing | success | Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/intel-Testing | success | Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-mellanox-Functional | success | Functional Testing PASS |
| ci/iol-mellanox-Functional | success | Functional Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/iol-broadcom-Functional | success | Functional Testing PASS |
| ci/iol-broadcom-Functional | success | Functional Testing PASS |
| ci/checkpatch | success | coding style OK |
On 1/18/2021 8:59 AM, Zhang,Alvin wrote: > From: Alvin Zhang <alvinx.zhang@intel.com> > > Since the patch '1848b117' has set the value of key in 'struct > rte_flow_action_rss' to NULL, the PMD cannot get the RSS key now. > > This patch sets offset and size of the key pointer as the first > parameter of the token 'key' and copies the start address of the > 'HEX' data to the location specified by the first parameter of > the token. > Can you please put the rte_flow command that enables reproducing this defect, it may help in the future? > Fixes: 1848b117cca1 ("app/testpmd: fix RSS key for flow API RSS rule") > Cc: stable@dpdk.org > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com> > --- > app/test-pmd/cmdline_flow.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index 585cab9..6eb46d3 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -3403,7 +3403,10 @@ static int comp_set_sample_index(struct context *, const struct token *, > .name = "key", > .help = "RSS hash key", > .next = NEXT(action_rss, NEXT_ENTRY(HEX)), > - .args = ARGS(ARGS_ENTRY_ARB(0, 0), > + .args = ARGS(ARGS_ENTRY_ARB > + (offsetof(struct action_rss_data, conf) + > + offsetof(struct rte_flow_action_rss, key), > + sizeof(((struct rte_flow_action_rss *)0)->key)), +1, it is required to write the address, and I confirm this enables getting 'key' value to the PMD. > ARGS_ENTRY_ARB > (offsetof(struct action_rss_data, conf) + > offsetof(struct rte_flow_action_rss, key_len), > @@ -6495,19 +6498,18 @@ static int comp_set_sample_index(struct context *, const struct token *, > if (ctx->objmask) > memset((uint8_t *)ctx->objmask + arg_data->offset, > 0xff, hexlen); > + > /* Save address if requested. */ > if (arg_addr->size) { > - memcpy((uint8_t *)ctx->object + arg_addr->offset, > - (void *[]){ > - (uint8_t *)ctx->object + arg_data->offset > - }, > - arg_addr->size); > + if (arg_addr->size < sizeof(void *)) > + goto error; Above two lines seems only actual change in this function, others are refactoring the assignment. 1) why this check required, I think we can ignore it. 2) What do you think to remove the refactoring to reduce the change to the actual fix? > + > + *(void **)((uint8_t *)ctx->object + arg_addr->offset) = > + (uint8_t *)ctx->object + arg_data->offset; > + > if (ctx->objmask) > - memcpy((uint8_t *)ctx->objmask + arg_addr->offset, > - (void *[]){ > - (uint8_t *)ctx->objmask + arg_data->offset > - }, > - arg_addr->size); > + *(void **)((uint8_t *)ctx->objmask + arg_addr->offset) = > + (uint8_t *)ctx->objmask + arg_data->offset; > } > return len; > error: >
Hi Ferruh, Thanks for your review. I will modify the patch in V2. Alvin > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Wednesday, January 20, 2021 3:15 AM > To: Zhang, AlvinX <alvinx.zhang@intel.com> > Cc: dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH] app/testpmd: fix RSS key > > On 1/18/2021 8:59 AM, Zhang,Alvin wrote: > > From: Alvin Zhang <alvinx.zhang@intel.com> > > > > Since the patch '1848b117' has set the value of key in 'struct > > rte_flow_action_rss' to NULL, the PMD cannot get the RSS key now. > > > > This patch sets offset and size of the key pointer as the first > > parameter of the token 'key' and copies the start address of the 'HEX' > > data to the location specified by the first parameter of the token. > > > > Can you please put the rte_flow command that enables reproducing this defect, > it may help in the future? > > > Fixes: 1848b117cca1 ("app/testpmd: fix RSS key for flow API RSS rule") > > Cc: stable@dpdk.org > > > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com> > > --- > > app/test-pmd/cmdline_flow.c | 24 +++++++++++++----------- > > 1 file changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > > index 585cab9..6eb46d3 100644 > > --- a/app/test-pmd/cmdline_flow.c > > +++ b/app/test-pmd/cmdline_flow.c > > @@ -3403,7 +3403,10 @@ static int comp_set_sample_index(struct context > *, const struct token *, > > .name = "key", > > .help = "RSS hash key", > > .next = NEXT(action_rss, NEXT_ENTRY(HEX)), > > - .args = ARGS(ARGS_ENTRY_ARB(0, 0), > > + .args = ARGS(ARGS_ENTRY_ARB > > + (offsetof(struct action_rss_data, conf) + > > + offsetof(struct rte_flow_action_rss, key), > > + sizeof(((struct rte_flow_action_rss *)0)->key)), > > > +1, it is required to write the address, and I confirm this enables > +getting > 'key' value to the PMD. > > > ARGS_ENTRY_ARB > > (offsetof(struct action_rss_data, conf) + > > offsetof(struct rte_flow_action_rss, key_len), @@ > -6495,19 > > +6498,18 @@ static int comp_set_sample_index(struct context *, const > struct token *, > > if (ctx->objmask) > > memset((uint8_t *)ctx->objmask + arg_data->offset, > > 0xff, hexlen); > > + > > /* Save address if requested. */ > > if (arg_addr->size) { > > - memcpy((uint8_t *)ctx->object + arg_addr->offset, > > - (void *[]){ > > - (uint8_t *)ctx->object + arg_data->offset > > - }, > > - arg_addr->size); > > + if (arg_addr->size < sizeof(void *)) > > + goto error; > > Above two lines seems only actual change in this function, others are refactoring > the assignment. > > 1) why this check required, I think we can ignore it. > 2) What do you think to remove the refactoring to reduce the change to the > actual fix? > > > + > > + *(void **)((uint8_t *)ctx->object + arg_addr->offset) = > > + (uint8_t *)ctx->object + arg_data->offset; > > + > > if (ctx->objmask) > > - memcpy((uint8_t *)ctx->objmask + arg_addr->offset, > > - (void *[]){ > > - (uint8_t *)ctx->objmask + arg_data->offset > > - }, > > - arg_addr->size); > > + *(void **)((uint8_t *)ctx->objmask + arg_addr->offset) = > > + (uint8_t *)ctx->objmask + arg_data->offset; > > } > > return len; > > error: > >
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c index 585cab9..6eb46d3 100644 --- a/app/test-pmd/cmdline_flow.c +++ b/app/test-pmd/cmdline_flow.c @@ -3403,7 +3403,10 @@ static int comp_set_sample_index(struct context *, const struct token *, .name = "key", .help = "RSS hash key", .next = NEXT(action_rss, NEXT_ENTRY(HEX)), - .args = ARGS(ARGS_ENTRY_ARB(0, 0), + .args = ARGS(ARGS_ENTRY_ARB + (offsetof(struct action_rss_data, conf) + + offsetof(struct rte_flow_action_rss, key), + sizeof(((struct rte_flow_action_rss *)0)->key)), ARGS_ENTRY_ARB (offsetof(struct action_rss_data, conf) + offsetof(struct rte_flow_action_rss, key_len), @@ -6495,19 +6498,18 @@ static int comp_set_sample_index(struct context *, const struct token *, if (ctx->objmask) memset((uint8_t *)ctx->objmask + arg_data->offset, 0xff, hexlen); + /* Save address if requested. */ if (arg_addr->size) { - memcpy((uint8_t *)ctx->object + arg_addr->offset, - (void *[]){ - (uint8_t *)ctx->object + arg_data->offset - }, - arg_addr->size); + if (arg_addr->size < sizeof(void *)) + goto error; + + *(void **)((uint8_t *)ctx->object + arg_addr->offset) = + (uint8_t *)ctx->object + arg_data->offset; + if (ctx->objmask) - memcpy((uint8_t *)ctx->objmask + arg_addr->offset, - (void *[]){ - (uint8_t *)ctx->objmask + arg_data->offset - }, - arg_addr->size); + *(void **)((uint8_t *)ctx->objmask + arg_addr->offset) = + (uint8_t *)ctx->objmask + arg_data->offset; } return len; error: