[dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial implementation of SW crypto device
>-Original Message-
>From: Jastrzebski, MichalX K
>Sent: Wednesday, September 14, 2016 10:48 AM
>To: Akhil Goyal ; Azarewicz, PiotrX T
>; Doherty, Declan
>; dev at dpdk.org
>Cc: Kerlin, MarcinX ; Mrozowicz, SlawomirX
>; Kobylinski, MichalX
>; Kulasek, TomaszX
>; Mrzyglod, DanielX T
>
>Subject: RE: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial implementation of
>SW crypto device
>
>> -Original Message-
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Akhil Goyal
>> Sent: Monday, September 12, 2016 1:17 PM
>> To: Azarewicz, PiotrX T ; Doherty,
>> Declan ; dev at dpdk.org
>> Cc: Kerlin, MarcinX ; Mrozowicz, SlawomirX
>> ; Kobylinski, MichalX
>> ; Kulasek, TomaszX
>> ; Mrzyglod, DanielX T
>>
>> Subject: Re: [dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial
>> implementation of SW crypto device
>>
>> On 8/26/2016 12:51 PM, Piotr Azarewicz wrote:
>>
>> > +/** Provide session for operation */ static struct
>> > +libcrypto_session * get_session(struct libcrypto_qp *qp, struct
>> > +rte_crypto_op *op) {
>> > + struct libcrypto_session *sess = NULL;
>> > +
>> > + if (op->sym->sess_type == RTE_CRYPTO_SYM_OP_WITH_SESSION) {
>> > + /* get existing session */
>> > + if (!unlikely(op->sym->session == NULL ||
>> > + op->sym->session->dev_type !=
>> > + RTE_CRYPTODEV_LIBCRYPTO_PMD))
>> > + sess = (struct libcrypto_session *)
>> > + op->sym->session->_private;
>> > + } else {
>> > + /* provide internal session */
>> > + void *_sess = NULL;
>> > +
>> > + if (!rte_mempool_get(qp->sess_mp, (void **)&_sess)) {
>> > + sess = (struct libcrypto_session *)
>> > + ((struct rte_cryptodev_sym_session *)_sess)
>> > + ->_private;
>> > +
>> > + if (unlikely(libcrypto_set_session_parameters(
>> > + sess, op->sym->xform) != 0)) {
>> > + rte_mempool_put(qp->sess_mp, _sess);
>> > + sess = NULL;
>> > + } else
>> > + op->sym->session = _sess;
>> > + }
>> > + }
>> > +
>> > + if (sess == NULL)
>> > + op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
>> > +
>> > + return sess;
>> > +}
>> > +
>> > +/*
>> > +
>> > +*--
>> > +
>> > + * Process Operations
>> > +
>> > +*--
>> > +
>> > + */
>> > +
>> > +/** Process standard libcrypto cipher encryption */ static int
>> > +process_libcrypto_cipher_encrypt(uint8_t *src, uint8_t *dst,
>> > + uint8_t *iv, uint8_t *key, int srclen,
>> > + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo) {
>> > + int dstlen, totlen;
>> > +
>> > + if (EVP_EncryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
>> > + goto process_cipher_encrypt_err;
>> [Akhil] this EVP_EncryptInit_ex() can be done for each session instead
>> of each packet. This will improve the performance. Also if there is
>> some change in the parameters later then it can be called again here
>> with the updated parameters only.
>> Same comment is for all cases (hmac, auth, etc)
>
>Hi Akhil,
>Thank You for this comment. We will check if this is possible.
>
>Michal
>
[S?awomir]
We have done some investigation:
- At first glance it is valuable to call this kind of function
only once per session.
- We done test: just remove the functions like EVP_XXXInitXXX
and we reduce packet processing time between 1% to 10%
depending of processed algorithm.
- We analyzed calling only once per session also functions
like EVP_XXXFinalXXX but it is not possible because
this kind of functions finalize cipher or authenticate process
and return result.
- The functions like EVP_XXXFinalXXX change context and
It is not possible to proper continue processing without
reinitialize the context. So after call functions EVP_XXXFinalXXX
we need to call EVP_XXXInitXXX for next operation.
In sum we can't do the proposed performance improvement.
>> > +
>> > + if (EVP_EncryptUpda
[dpdk-dev] [PATCH v4 4/5] app/test: added big data GMAC test for libcrypto
>-Original Message- >From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] >Sent: Friday, September 30, 2016 7:19 PM >To: Mrozowicz, SlawomirX ; Azarewicz, >PiotrX T >Cc: dev at dpdk.org; Dai, Wei >Subject: Re: [dpdk-dev] [PATCH v4 4/5] app/test: added big data GMAC test >for libcrypto > >2016-09-30 18:32, Slawomir Mrozowicz: >> This patch add big data AES-GMAC test for libcrypto PMD. >> >> Signed-off-by: Piotr Azarewicz >> --- >> app/test/test_cryptodev.c | 18 +- >> app/test/test_cryptodev_gcm_test_vectors.h | 8245 >+++- >> 2 files changed, 8242 insertions(+), 21 deletions(-) > >The test data are really too big. >Is it possible to generate them as Wei Dai did for LPM? > http://dpdk.org/patch/16175 > http://dpdk.org/patch/16253 Yes it is possible. We will prepare new patch set with the changes which you proposed. S?awek
[dpdk-dev] [PATCH v5] eal: out-of-bounds write
Hi Thomas, As I understand Sergio suggested to come back to the solution similar to v1. Could you comment or better take decision which solution should be applied, please. Best Regards, S?awomir >-Original Message- >From: Gonzalez Monroy, Sergio >Sent: Monday, June 20, 2016 1:29 PM >To: Thomas Monjalon >Cc: Mrozowicz, SlawomirX ; >dev at dpdk.org; david.marchand at 6wind.com >Subject: Re: [dpdk-dev] [PATCH v5] eal: out-of-bounds write > >On 20/06/2016 11:09, Thomas Monjalon wrote: >> 2016-06-20 10:38, Sergio Gonzalez Monroy: >>> On 20/06/2016 10:14, Thomas Monjalon wrote: >>>>> + RTE_LOG(ERR, EAL, >>>>> + "All memory segments exhausted by IVSHMEM. " >>>> There is no evidence that it is related to IVSHMEM. >>>> "Not enough memory segments." would be more appropriate. >>> Actually we would hit this issue when all memsegs have been used by >IVSHMEM. >>> So I think the message is accurate. >> I think it's saner to avoid mixing "potential root cause of a use >> case" and "accurate description of the error". >> One day, the root cause could be different and the message will become >wrong. >> Here there is not enough memory segment. >> > >Right. >So the whole point of doing the check before the loop was to display the error >message with its specific cause. > >I would think that if the code changes and the message is not accurate then it >should also be updated. > >So if folks prefer a more generic error message probably we don't need the >check before the loop and just change the check condition inside the loop that >would end up printing the generic error message (after the loop). > >Basically v1 would do that. >http://dpdk.org/dev/patchwork/patch/12241/ > >Sergio
[dpdk-dev] [PATCH] examples/ipsec-secgw: Calling risky function
>-Original Message-
>From: Gonzalez Monroy, Sergio
>Sent: Tuesday, June 7, 2016 10:15 AM
>To: Mrozowicz, SlawomirX
>Cc: dev at dpdk.org
>Subject: Re: [PATCH] examples/ipsec-secgw: Calling risky function
>
>On 07/06/2016 09:58, Slawomir Mrozowicz wrote:
>> lrand48 should not be used for security related applications, as
>> linear congruential algorithms are too easy to break.
>> Used a compliant random number generator /dev/urandom.
>>
>> Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample
>> application") Coverity ID 124558
>>
>> Signed-off-by: Slawomir Mrozowicz
>> ---
>
>I understand that lrand48 is not crypto secure, but this fix will kill
>performance.
>
>I already have a solution for this issue to be included in the next IPSec patch
>set that will also add support for GCM/CTR modes.
>
>Sergio
Thanks for your reply.
So for now I propose to set this problem as intentional in the Coverity tool.
S?awomir
[dpdk-dev] [PATCH] rte mempool: division or modulo by zero
Hi Olivier,
I try to marge my change CID 13234 with your patch 12057.
Can you tell me which is the base commit to apply the patch.
I think that I should apply your patches starting from 12834.
Regards,
Slawomir
>-Original Message-
>From: Olivier Matz [mailto:olivier.matz at 6wind.com]
>Sent: Monday, May 16, 2016 11:23 AM
>To: Mrozowicz, SlawomirX
>Cc: dev at dpdk.org
>Subject: Re: [PATCH] rte mempool: division or modulo by zero
>
>Hi Slawomir,
>
>On 05/12/2016 02:46 PM, Slawomir Mrozowicz wrote:
>> Fix issue reported by Coverity.
>>
>> Coverity ID 13243: Division or modulo by zero In function call
>> rte_mempool_xmem_size, division by expression total_size which may be
>> zero has undefined behavior.
>>
>> Fixes: 148f963fb532 ("xen: core library changes")
>>
>> Signed-off-by: Slawomir Mrozowicz
>> ---
>> lib/librte_mempool/rte_mempool.c | 18 +++---
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c
>> b/lib/librte_mempool/rte_mempool.c
>> index f8781e1..01668c1 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -327,15 +327,19 @@ rte_mempool_calc_obj_size(uint32_t elt_size,
>> uint32_t flags, size_t rte_mempool_xmem_size(uint32_t elt_num,
>> size_t elt_sz, uint32_t pg_shift) {
>> -size_t n, pg_num, pg_sz, sz;
>> +size_t n, pg_num, pg_sz;
>> +size_t sz = 0;
>>
>> -pg_sz = (size_t)1 << pg_shift;
>> +if (elt_sz > 0) {
>> +pg_sz = (size_t)1 << pg_shift;
>> +n = pg_sz / elt_sz;
>>
>> -if ((n = pg_sz / elt_sz) > 0) {
>> -pg_num = (elt_num + n - 1) / n;
>> -sz = pg_num << pg_shift;
>> -} else {
>> -sz = RTE_ALIGN_CEIL(elt_sz, pg_sz) * elt_num;
>> +if (n > 0) {
>> +pg_num = (elt_num + n - 1) / n;
>> +sz = pg_num << pg_shift;
>> +} else {
>> +sz = RTE_ALIGN_CEIL(elt_sz, pg_sz) * elt_num;
>> +}
>> }
>>
>> return sz;
>>
>
>I think it would be clearer (either for the patch and the code) to avoid an
>additional indent, and do something like that:
>
> size_t
> rte_mempool_xmem_size(uint32_t elt_num, size_t elt_sz,
> uint32_t pg_shift)
> {
> if (elt_sz == 0)
> return 0;
>
> /* same code as before */
>
>It will also facilitate the merge with
>http://patchwork.dpdk.org/dev/patchwork/patch/12057/
>
>Could you please submit a v2 with this logic?
>
>Thanks,
>Olivier
[dpdk-dev] test-pmd: Free of address-of expression
Hi, Noticed is that in the file: app/test-pmd/mempool.c using of the function munmap() could cause a problem. Coverity static code analyzer provide error (CID 13184) in line 158: munmap frees incorrect pointer uv. I noticed information on the net: https://www.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.bpxbd00/mumap.htm "If addr is not the address of a mapping established by a prior call to mmap(), the behavior is undefined" I have analyzed the code and I have done some test with gcc. It seems that it is possible to free subrange of the mapping memory. In the mempool.c code the address is calculated independently. Anyway in my opinion the address variable uv is calculated correctly. So we should classify this issue as a False Positive. Please accept the conclusion. Slawomir Mrozowicz Sii Engineer Delivering outsourced services to Intel Intel Technology Poland sp. z o.o. - KRS 101882 - ul. Slowackiego 173, 80-298 Gdansk
[dpdk-dev] [PATCH v3] examples/qos_meter: fix unchecked return value
>-Original Message-
>From: Dumitrescu, Cristian
>Sent: Wednesday, May 11, 2016 7:57 PM
>To: Mrozowicz, SlawomirX
>Cc: dev at dpdk.org; Singh, Jasvinder
>Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
>> -----Original Message-----
>> From: Mrozowicz, SlawomirX
>> Sent: Wednesday, May 11, 2016 10:15 AM
>> To: Dumitrescu, Cristian
>> Cc: dev at dpdk.org; Singh, Jasvinder
>> Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>>
>>
>>
>> >-Original Message-
>> >From: Dumitrescu, Cristian
>> >Sent: Tuesday, May 10, 2016 7:42 PM
>> >To: Mrozowicz, SlawomirX
>> >Cc: dev at dpdk.org; Singh, Jasvinder
>> >Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return
>> >value
>> >
>> >
>> >
>> >> -Original Message-
>> >> From: Mrozowicz, SlawomirX
>> >> Sent: Monday, May 9, 2016 9:38 AM
>> >> To: Dumitrescu, Cristian
>> >> Cc: dev at dpdk.org; Singh, Jasvinder ;
>> >> Mrozowicz, SlawomirX
>> >> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
>> >>
>> >> Fix issue reported by Coverity.
>> >>
>> >> Coverity ID 30693: Unchecked return value
>> >> check_return: Calling rte_meter_srtcm_config without checking
>> >> return value.
>> >>
>> >> Fixes: e6541fdec8b2 ("meter: initial import")
>> >>
>> >> Signed-off-by: Slawomir Mrozowicz
>> >> ---
>> >> examples/qos_meter/main.c | 15 ++-
>> >> examples/qos_meter/main.h | 2 +-
>> >> 2 files changed, 11 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
>> >> index b968b00..7c69606 100644
>> >> --- a/examples/qos_meter/main.c
>> >> +++ b/examples/qos_meter/main.c
>> >> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
>> >app_trtcm_params[]
>> >> = {
>> >>
>> >> FLOW_METER app_flows[APP_FLOWS_MAX];
>> >>
>> >> -static void
>> >> +static int
>> >> app_configure_flow_table(void)
>> >> {
>> >> uint32_t i, j;
>> >> + int ret = 0;
>> >>
>> >> - for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
>> >> RTE_DIM(PARAMS)){
>> >> - FUNC_CONFIG(_flows[i], [j]);
>> >> - }
>> >> + for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
>> >> + i ++, j = (j + 1) % RTE_DIM(PARAMS))
>> >> + ret = FUNC_CONFIG(_flows[i], [j]);
>> >> +
>> >> + return ret;
>> >> }
>> >
>> >This is only returns the configuration status for the last flow and
>> >leaves undetected an error for any other flow. Why not check the
>> >status for each flow and return an error on first occurrence?
>> >For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
>> >
>>
>> This code check status of the function FUNC_CONFIG for each flow and
>> return an error on first occurrence. Rest of functions FUNC_CONFIG are
>> not called. See terminate condition of the loop.
>>
>
>Where is the status of FUNC_CONFIG checked exactly? I cannot see any check
>in your code. I can only see returning the status code for the last call of
>this
>function in the for loop. I was expecting a check such as: if (ret) return ret.
>
Look at the loop terminate conditions:
i < APP_FLOWS_MAX && ret == 0
Program terminate the loop if the ret variable is differ then zero.
It means that program terminate if the last status of FUNC_CONFIG is an error.
>> >>
>> >> static inline void
>> >> @@ -381,7 +384,9 @@ main(int argc, char **argv)
>> >> rte_eth_promiscuous_enable(port_tx);
>> >>
>> >> /* App configuration */
>> >> - app_configure_flow_table();
>> >> + ret = app_configure_flow_table();
>> >> + if (ret < 0)
>> >> + rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
>> >>
>> >> /* Launch per-lcore init on every lcore */
>> >> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff -
>> -
>> >git
>> >> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
>> >> 530bf69..54867dc 100644
>> >> --- a/examples/qos_meter/main.h
>> >> +++ b/examples/qos_meter/main.h
>> >> @@ -51,7 +51,7 @@ enum policer_action
>> >> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
>> >APP_MODE
>> >> == APP_MODE_FWD
>> >>
>> >> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id,
>> >> pkt_len=pkt_len, time=time -#define FUNC_CONFIG(a,b)
>> >> +#define FUNC_CONFIG(a, b) 0
>> >> #define PARAMS app_srtcm_params
>> >> #define FLOW_METER int
>> >>
>> >> --
>> >> 1.9.1
[dpdk-dev] [PATCH v3] examples/qos_meter: fix unchecked return value
>-Original Message-
>From: Dumitrescu, Cristian
>Sent: Tuesday, May 10, 2016 7:42 PM
>To: Mrozowicz, SlawomirX
>Cc: dev at dpdk.org; Singh, Jasvinder
>Subject: RE: [PATCH v3] examples/qos_meter: fix unchecked return value
>
>
>
>> -----Original Message-----
>> From: Mrozowicz, SlawomirX
>> Sent: Monday, May 9, 2016 9:38 AM
>> To: Dumitrescu, Cristian
>> Cc: dev at dpdk.org; Singh, Jasvinder ;
>> Mrozowicz, SlawomirX
>> Subject: [PATCH v3] examples/qos_meter: fix unchecked return value
>>
>> Fix issue reported by Coverity.
>>
>> Coverity ID 30693: Unchecked return value
>> check_return: Calling rte_meter_srtcm_config without checking return
>> value.
>>
>> Fixes: e6541fdec8b2 ("meter: initial import")
>>
>> Signed-off-by: Slawomir Mrozowicz
>> ---
>> examples/qos_meter/main.c | 15 ++-
>> examples/qos_meter/main.h | 2 +-
>> 2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/examples/qos_meter/main.c b/examples/qos_meter/main.c
>> index b968b00..7c69606 100644
>> --- a/examples/qos_meter/main.c
>> +++ b/examples/qos_meter/main.c
>> @@ -133,14 +133,17 @@ struct rte_meter_trtcm_params
>app_trtcm_params[]
>> = {
>>
>> FLOW_METER app_flows[APP_FLOWS_MAX];
>>
>> -static void
>> +static int
>> app_configure_flow_table(void)
>> {
>> uint32_t i, j;
>> +int ret = 0;
>>
>> -for (i = 0, j = 0; i < APP_FLOWS_MAX; i ++, j = (j + 1) %
>> RTE_DIM(PARAMS)){
>> -FUNC_CONFIG(_flows[i], [j]);
>> -}
>> +for (i = 0, j = 0; i < APP_FLOWS_MAX && ret == 0;
>> +i ++, j = (j + 1) % RTE_DIM(PARAMS))
>> +ret = FUNC_CONFIG(_flows[i], [j]);
>> +
>> +return ret;
>> }
>
>This is only returns the configuration status for the last flow and leaves
>undetected an error for any other flow. Why not check the status for each
>flow and return an error on first occurrence?
>For (...){ret = FUNC_CONFIG(...); if (ret) return ret;}
>
This code check status of the function FUNC_CONFIG for each flow and return an
error on first occurrence. Rest of functions FUNC_CONFIG are not called. See
terminate condition of the loop.
>>
>> static inline void
>> @@ -381,7 +384,9 @@ main(int argc, char **argv)
>> rte_eth_promiscuous_enable(port_tx);
>>
>> /* App configuration */
>> -app_configure_flow_table();
>> +ret = app_configure_flow_table();
>> +if (ret < 0)
>> +rte_exit(EXIT_FAILURE, "Invalid configure flow table\n");
>>
>> /* Launch per-lcore init on every lcore */
>> rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER); diff --
>git
>> a/examples/qos_meter/main.h b/examples/qos_meter/main.h index
>> 530bf69..54867dc 100644
>> --- a/examples/qos_meter/main.h
>> +++ b/examples/qos_meter/main.h
>> @@ -51,7 +51,7 @@ enum policer_action
>> policer_table[e_RTE_METER_COLORS][e_RTE_METER_COLORS] = #if
>APP_MODE
>> == APP_MODE_FWD
>>
>> #define FUNC_METER(a,b,c,d) color, flow_id=flow_id, pkt_len=pkt_len,
>> time=time -#define FUNC_CONFIG(a,b)
>> +#define FUNC_CONFIG(a, b) 0
>> #define PARAMS app_srtcm_params
>> #define FLOW_METER int
>>
>> --
>> 1.9.1
[dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation
>-Original Message-
>From: Dumitrescu, Cristian
>Sent: Thursday, April 28, 2016 1:16 PM
>To: Jastrzebski, MichalX K ; Zhang, Roy Fan
>; Singh, Jasvinder
>Cc: dev at dpdk.org; Mrozowicz, SlawomirX
>Subject: RE: [PATCH v3] examples/qos_sched: fix bad bit shift operation
>
>
>
>> -Original Message-
>> From: Jastrzebski, MichalX K
>> Sent: Thursday, April 21, 2016 2:08 PM
>> To: Dumitrescu, Cristian ; Zhang, Roy
>> Fan ; Singh, Jasvinder
>>
>> Cc: dev at dpdk.org; Mrozowicz, SlawomirX
>
>> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation
>>
>> From: Slawomir Mrozowicz
>>
>> Fix issue reported by Coverity.
>>
>> Coverity ID 30690: Bad bit shift operation
>> large_shift: In expression 1ULL << i, left shifting by more than 63
>> bits has undefined behavior. The shift amount, i, is as much as 127.
>>
>> Fixes: de3cfa2c9823 ("sched: initial import")
>>
>> Signed-off-by: Slawomir Mrozowicz
>> ---
>> examples/qos_sched/args.c | 84 +-
>-
>>
>> 1 file changed, 52 insertions(+), 32 deletions(-)
>>
>> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
>> index 3e7fd08..cd077ba 100644
>> --- a/examples/qos_sched/args.c
>> +++ b/examples/qos_sched/args.c
>> @@ -53,7 +53,7 @@
>>
>> static uint32_t app_master_core = 1;
>> static uint32_t app_numa_mask;
>> -static uint64_t app_used_core_mask = 0;
>> +static int app_used_core_mask[RTE_MAX_LCORE];
Changed type of the app_used_core_mask variable to store up to RTE_MAX_LCORE
cores information.
>> static uint64_t app_used_port_mask = 0; static uint64_t
>> app_used_rx_port_mask = 0; static uint64_t app_used_tx_port_mask = 0;
>> @@ -115,22 +115,23 @@ static inline int str_is(const char *str, const char
>*is)
>> return strcmp(str, is) == 0;
>> }
>>
>> -/* returns core mask used by DPDK */
>> -static uint64_t
>> -app_eal_core_mask(void)
>> +/* compare used core with eal configuration,
>> +returns:
>> +1 if equal
>> +0 if differ */
>> +static int
>> +app_eal_core_check(void)
>> {
>> -uint32_t i;
>> -uint64_t cm = 0;
>> +uint16_t i;
>> +int ret = 1;
>> struct rte_config *cfg = rte_eal_get_configuration();
>>
>> -for (i = 0; i < RTE_MAX_LCORE; i++) {
>> -if (cfg->lcore_role[i] == ROLE_RTE)
>> -cm |= (1ULL << i);
>> +for (i = 0; i < RTE_MAX_LCORE && ret; i++) {
>> +if ((cfg->lcore_role[i] == ROLE_RTE) !=
>> app_used_core_mask[i])
>> +ret = 0;
>> }
>>
>> -cm |= (1ULL << cfg->master_lcore);
>> -
>> -return cm;
>> +return ret;
>> }
>>
Added tool function app_eal_core_check() to check compatibility used cores with
information stored in configuration file. The function is used below.
Removed not used function app_eal_core_mask()
>>
>> @@ -292,14 +293,9 @@ app_parse_flow_conf(const char *conf_str)
>> app_used_tx_port_mask |= mask;
>> app_used_port_mask |= mask;
>>
>> -mask = 1lu << pconf->rx_core;
>> -app_used_core_mask |= mask;
>> -
>> -mask = 1lu << pconf->wt_core;
>> -app_used_core_mask |= mask;
>> -
>> -mask = 1lu << pconf->tx_core;
>> -app_used_core_mask |= mask;
>> +app_used_core_mask[pconf->rx_core] = 1;
>> +app_used_core_mask[pconf->wt_core] = 1;
>> +app_used_core_mask[pconf->tx_core] = 1;
>>
Change method of set the mask on each used core according to change mask type.
>> nb_pfc++;
>>
>> @@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv)
>> int option_index;
>> const char *optname;
>> char *prgname = argv[0];
>> -uint32_t i, nb_lcores;
>> +uint16_t i, j, k, nb_lcores;
>>
>> static struct option lgopts[] = {
>> { "pfc", 1, 0, 0 },
>> @@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv)
>> { NULL, 0, 0, 0 }
>> };
>>
>> +for (i = 0; i < RTE_MAX_LCORE; i++)
>> +app_used_core_mask[i] = 0;
>> +
Set initial value of the mask.
>> /* initialize EAL first */
>> ret = rte_eal_init(argc, argv);
>> if (ret < 0)
>> @@ -436,19 +435,40 @@ app_parse_args(int argc, char **argv)
>>
[dpdk-dev] [PATCH] lpm: unchecked return value
>-Original Message-
>From: Richardson, Bruce
>Sent: Tuesday, May 3, 2016 4:34 PM
>To: Mrozowicz, SlawomirX
>Cc: dev at dpdk.org
>Subject: Re: [PATCH] lpm: unchecked return value
>
>On Wed, Apr 27, 2016 at 02:52:34PM +0200, Slawomir Mrozowicz wrote:
>> Fix issue reported by Coverity.
>>
>> Coverity ID 13205: Unchecked return value Unchecked return value
>> check_return: Calling rte_lpm6_add without checking return value
>> Fixes: 5c510e13a9cb ("lpm: add IPv6 support")
>>
>> Signed-off-by: Slawomir Mrozowicz
>> ---
>> lib/librte_lpm/rte_lpm6.c | 10 ++
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
>> index ba4353c..f4db3fa 100644
>> --- a/lib/librte_lpm/rte_lpm6.c
>> +++ b/lib/librte_lpm/rte_lpm6.c
>> @@ -749,6 +749,7 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t *ip,
>uint8_t depth)
>> int32_t rule_to_delete_index;
>> uint8_t ip_masked[RTE_LPM6_IPV6_ADDR_SIZE];
>> unsigned i;
>> +int status = 0;
>>
>> /*
>> * Check input arguments.
>> @@ -790,12 +791,13 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t
>*ip, uint8_t depth)
>> * Add every rule again (except for the one that was removed from
>> * the rules table).
>> */
>> -for (i = 0; i < lpm->used_rules; i++) {
>> -rte_lpm6_add(lpm, lpm->rules_tbl[i].ip, lpm-
>>rules_tbl[i].depth,
>> -lpm->rules_tbl[i].next_hop);
>> +for (i = 0; i < lpm->used_rules && status >= 0; i++) {
>> +status = rte_lpm6_add(
>> +lpm, lpm->rules_tbl[i].ip, lpm->rules_tbl[i].depth,
>> +lpm->rules_tbl[i].next_hop);
>> }
>>
>> -return 0;
>> +return status;
>> }
>
>Hi,
>
>I'm not sure that this patch is actually necessary, as I'm not sure that the
>lpm6_add calls can fail in this instance. Looking through the code, this
>function
>deletes the rule and then clears the actual lpm lookup tables before re-adding
>all other routes to it again. The only error condition that could be returned,
>that I can see, is -ENOSPC, which should never occur here since the original
>rules fitted in the first place.
>
>If it was possible to fail, then I think we would have a worse problem, in that
>deleting a single rule has wiped out our lpm table and left it in an
>inconsistent
>state, so the error handling probably needs to be better than just quitting.
>
>Finally, one other thing I spot looking through the code, is that there seems
>to
>be a worrying set of calls between add and delete. If the add function fails,
>then it calls delete which in turn will call add again, etc. etc. This may all
>work
>correctly, but it seems fragile and error prone to me - especially if we allow
>calls from one to another to fail.
>
>This looks like it might need some further examination to verify what the
>possible failure cases are and what happens in each scenario.
>
>Regards,
>/Bruce
Hi Bruce,
In my opinion the worst-case scenario should be take into account. If function
like rte_lpm6_add() returns false then it should be handled.
Anyway I agree with you that if the function fail then we have serious problem.
I see two problems:
1. Code construction: calls between function rte_lpm6_add() and
rte_lpm6_delete(). As you said it should be examined.
2. How we should handle situation if the rules table are not reconstructed
after delete operation.
I propose to add new issue in ClearQuest to proceed solve the problems because
there are extend the original issue (CID 13205 Unchecked return value) from
Coverity.
Regards,
S?awomir
[dpdk-dev] [PATCH] eal: out-of-bounds write
>-Original Message-
>From: Gonzalez Monroy, Sergio
>Sent: Tuesday, April 26, 2016 11:44 AM
>To: Richardson, Bruce ; Mrozowicz, SlawomirX
>
>Cc: david.marchand at 6wind.com; dev at dpdk.org
>Subject: Re: [dpdk-dev] [PATCH] eal: out-of-bounds write
>
>On 26/04/2016 09:53, Bruce Richardson wrote:
>> On Tue, Apr 26, 2016 at 09:44:47AM +0200, Slawomir Mrozowicz wrote:
>>> Fix issue reported by Coverity.
>>>
>>> Coverity ID 13282: Out-of-bounds write
>>> overrun-local: Overrunning array mcfg->memseg of 256 44-byte elements
>>> at element index 257 using index j.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>>
>>> Signed-off-by: Slawomir Mrozowicz
>>> ---
>>> lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> index 5b9132c..1e737e4 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>>> @@ -1333,7 +1333,7 @@ rte_eal_hugepage_init(void)
>>>
>>> if (new_memseg) {
>>> j += 1;
>>> - if (j == RTE_MAX_MEMSEG)
>>> + if (j >= RTE_MAX_MEMSEG)
>>> break;
>>>
>>> mcfg->memseg[j].phys_addr = hugepage[i].physaddr;
>>> --
>> This does appear to be a valid fix for the issue. However, looking at
>> the code, it appears that the only way we could actually hit the
>> problem is if j == RTE_MAX_MEMSEG on exiting the previous loop. Would
>> a check there be a better fix for this issue (or perhaps we want both fixes).
>>
>> Thoughts?
>
>It doesn't make sense to go into the loop if we don't have free memsegs.
>Either way we should print the error indicating that we reached
>MAX_MEMSEG.
>
>Sergio
>
It is possible to add additional checking available memseg before the loop.
In this case it will be checked twice before loop and inside loop.
In my opinion it is not necessary.
Anyway it is valuable to add in line 1336 error message if the MAX_MEMSEG is
reached.
S?awomir
>
>> /Bruce
