Re: [dpdk-stable] [dpdk-dev] [PATCH v4 03/18] bus/pci: replace strncpy dangerous code

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 03/18] bus/pci: replace strncpy dangerous code

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 03/18] bus/pci: replace strncpy dangerous code
> 
> In function ‘pci_get_kernel_driver_by_path’,
>     inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/
> 	drivers/bus/pci/linux/pci.c:317:8:
> /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error:
> ‘strncpy’ specified bound depends on the length of the source argument [-
> Werror=stringop-overflow=]
>    strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Forgot to say that this needs fixes lines and cc stable.

Fixes: d9a8cd9595f2 ("pci: add kernel driver type")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 04/18] bus/dpaa: solve inconsistent struct alignment

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 04/18] bus/dpaa: solve inconsistent struct
> alignment
> 
> The actual descriptor for qm_mr_entry is 64-byte aligned.
> 
> But the original code plays a trick, and puts a u8 common to the three descriptor
> subtypes in the union afterwards outside their structure definitions.
> 
> Unfortunately since they compose a struct qm_fd with alignment 8, this trick
> destroys the ability of the compiler to understand what has happened, resulting
> in this kind of
> problem:
> 
> /home/agreen/projects/dpdk/drivers/bus/dpaa/include/
> fsl_qman.h:354:3: error: alignment 1 of ‘struct <anonymous>’
> is less than 8 [-Werror=packed-not-aligned]
>    } __packed dcern;
> 
> on gcc 8 / Fedora 28 out of the box.
> 
> This patch moves the u8 verb into the structure definitions composed into the
> union, so the alignment of the parent struct containing the alignment 8 object
> can also be seen to be alignment 8 by the compiler.  Uses of .verb are fixed up to
> use .ern.verb (the same offset of +0 inside all the structs in the union).
> 
> The final struct layout should be unchanged.
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Missing fixes line and CC stable:

Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
Fixes: f6fadc3e6310 ("bus/dpaa: add QMAN interface driver")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:45 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> 
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function
> ‘nfp_pf_pci_probe’:
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3160:
> 23: error: ‘%s’ directive writing up to 99 bytes into a region of size 76 [-
> Werror=format-overflow=]
>   sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
> 
> Note fw_buf still has to increase somewhat even after restricting serial[], since
> otherwise:
> 
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function
> ‘nfp_pf_pci_probe’:
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:23:
> error: ‘%s’ directive writing up to 99 bytes into a region of size 76 [-
> Werror=format-overflow=]
>   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
>                        ^~
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
>   err = nfp_fw_upload(dev, nsp, card_desc);
>                                 ~~~~~~~~~
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
> note: ‘sprintf’ output between 25 and 124 bytes into a destination of size 100
>   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable.

Fixes: 896c265ef954 ("net/nfp: use new CPP interface")
Cc: stable@dpdk.org

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow

[De Lara Guarch, Pablo]

Hi,

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Friday, May 11, 2018 9:58 AM
> To: 'Andy Green' <andy@warmcat.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> > Sent: Friday, May 11, 2018 2:45 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v4 02/18] net/nfp: solve buffer overflow
> >
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function
> > ‘nfp_pf_pci_probe’:
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3160:
> > 23: error: ‘%s’ directive writing up to 99 bytes into a region of size
> > 76 [- Werror=format-overflow=]
> >   sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
> >
> > Note fw_buf still has to increase somewhat even after restricting
> > serial[], since
> > otherwise:
> >
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In function
> > ‘nfp_pf_pci_probe’:
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:23:
> > error: ‘%s’ directive writing up to 99 bytes into a region of size 76
> > [- Werror=format-overflow=]
> >   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> >                        ^~
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
> >   err = nfp_fw_upload(dev, nsp, card_desc);
> >                                 ~~~~~~~~~
> > /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
> > note: ‘sprintf’ output between 25 and 124 bytes into a destination of size 100
> >   sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> >
> > Signed-off-by: Andy Green <andy@warmcat.com>
> 
> Missing fixes line and CC stable.
> 
> Fixes: 896c265ef954 ("net/nfp: use new CPP interface")
> Cc: stable@dpdk.org
> 
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Actually, this does not need to be backported to stable, as it was merged in this release.
Sorry about the noise.

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 08/18] net/nfp: don't memcpy out of source range

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 08/18] net/nfp: don't memcpy out of source
> range
> 
> /home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:669:2:
> error: ‘memcpy’ forming offset [5, 6] is out of the bounds [0, 4] of object ‘tmp’
> with type ‘uint32_t’ {aka ‘unsigned int’} [-Werror=array-bounds]  memcpy(&hw-
> >mac_addr[0], &tmp, sizeof(struct ether_addr));
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Missing fixes line and CC stable.

Fixes: e6decee38209 ("net/nfp: use random MAC address if not configured")

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/qede/base/ecore_int.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/qede/base/ecore_int.c
> b/drivers/net/qede/base/ecore_int.c
> index f43781ba4..d9e22b5ed 100644
> --- a/drivers/net/qede/base/ecore_int.c
> +++ b/drivers/net/qede/base/ecore_int.c
> @@ -6,6 +6,8 @@
>   * See LICENSE.qede_pmd for copyright and licensing details.
>   */
> 
> +#include <rte_string_fns.h>
> +
>  #include "bcm_osal.h"
>  #include "ecore.h"
>  #include "ecore_spq.h"
> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>  							      p_aeu->bit_name,
>  							      num);
>  					else
> -						OSAL_STRNCPY(bit_name,
> -							     p_aeu->bit_name,
> -							     30);
> +						strlcpy(bit_name,
> +							p_aeu->bit_name,
> +							sizeof(bit_name));
> 
>  					/* We now need to pass bitmask in its
>  					 * correct position.

I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
modify the macro to use strlcpy, so we avoid further uses of that strlcpy.

However, this modifies base driver code, so it is up to the maintainers to make that decision.
(CC'ing maintainers here).

Also, missing fixes line and CC stable.

Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[Andy Green]

On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/qede/base/ecore_int.c |    8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/qede/base/ecore_int.c
>> b/drivers/net/qede/base/ecore_int.c
>> index f43781ba4..d9e22b5ed 100644
>> --- a/drivers/net/qede/base/ecore_int.c
>> +++ b/drivers/net/qede/base/ecore_int.c
>> @@ -6,6 +6,8 @@
>>    * See LICENSE.qede_pmd for copyright and licensing details.
>>    */
>>
>> +#include <rte_string_fns.h>
>> +
>>   #include "bcm_osal.h"
>>   #include "ecore.h"
>>   #include "ecore_spq.h"
>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>   							      p_aeu->bit_name,
>>   							      num);
>>   					else
>> -						OSAL_STRNCPY(bit_name,
>> -							     p_aeu->bit_name,
>> -							     30);
>> +						strlcpy(bit_name,
>> +							p_aeu->bit_name,
>> +							sizeof(bit_name));
>>
>>   					/* We now need to pass bitmask in its
>>   					 * correct position.
> 
> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> 
> However, this modifies base driver code, so it is up to the maintainers to make that decision.
> (CC'ing maintainers here).

There's no value for any OSAL_* that simply defines itself to be the 
same as the direct api, as does OSAL_STRNCPY.

It's better to just remove any OSAL_* that calls straight through since 
all it does is obfuscate what the code does, for no benefit.

> Also, missing fixes line and CC stable.
> 
> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")

The idea of this "Fixes" thing is to look up in git blame what is being 
edited is it?  If so what's the benefit of that over using git if you 
ever want to know that?

-Andy

> Cc: stable@dpdk.org
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 11:48 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> 
> 
> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >> Sent: Friday, May 11, 2018 2:46 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >> Signed-off-by: Andy Green <andy@warmcat.com>
> >> ---
> >>   drivers/net/qede/base/ecore_int.c |    8 +++++---
> >>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/qede/base/ecore_int.c
> >> b/drivers/net/qede/base/ecore_int.c
> >> index f43781ba4..d9e22b5ed 100644
> >> --- a/drivers/net/qede/base/ecore_int.c
> >> +++ b/drivers/net/qede/base/ecore_int.c
> >> @@ -6,6 +6,8 @@
> >>    * See LICENSE.qede_pmd for copyright and licensing details.
> >>    */
> >>
> >> +#include <rte_string_fns.h>
> >> +
> >>   #include "bcm_osal.h"
> >>   #include "ecore.h"
> >>   #include "ecore_spq.h"
> >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >>   							      p_aeu->bit_name,
> >>   							      num);
> >>   					else
> >> -						OSAL_STRNCPY(bit_name,
> >> -							     p_aeu->bit_name,
> >> -							     30);
> >> +						strlcpy(bit_name,
> >> +							p_aeu->bit_name,
> >> +							sizeof(bit_name));
> >>
> >>   					/* We now need to pass bitmask in its
> >>   					 * correct position.
> >
> > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> > modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> >
> > However, this modifies base driver code, so it is up to the maintainers to make
> that decision.
> > (CC'ing maintainers here).
> 
> There's no value for any OSAL_* that simply defines itself to be the same as the
> direct api, as does OSAL_STRNCPY.
> 
> It's better to just remove any OSAL_* that calls straight through since all it does
> is obfuscate what the code does, for no benefit.

I agree. Since this is modifying base driver code,
the maintainers can decide what to do with this.

> 
> > Also, missing fixes line and CC stable.
> >
> > Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
> 
> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
> If so what's the benefit of that over using git if you ever want to know that?
> 

This is important mainly to see which releases needed this patch backported.
I am replying to your patches with this info, to save you some time
(I know you are feeling the pain of this overhead :P).

> -Andy
> 
> > Cc: stable@dpdk.org
> >

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[Andy Green]

On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: Andy Green [mailto:andy@warmcat.com]
>> Sent: Friday, May 11, 2018 11:48 AM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
>> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
>> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>>
>>
>> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>>>> Sent: Friday, May 11, 2018 2:46 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
>>>> constant and NUL
>>>>
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>> ---
>>>>    drivers/net/qede/base/ecore_int.c |    8 +++++---
>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/qede/base/ecore_int.c
>>>> b/drivers/net/qede/base/ecore_int.c
>>>> index f43781ba4..d9e22b5ed 100644
>>>> --- a/drivers/net/qede/base/ecore_int.c
>>>> +++ b/drivers/net/qede/base/ecore_int.c
>>>> @@ -6,6 +6,8 @@
>>>>     * See LICENSE.qede_pmd for copyright and licensing details.
>>>>     */
>>>>
>>>> +#include <rte_string_fns.h>
>>>> +
>>>>    #include "bcm_osal.h"
>>>>    #include "ecore.h"
>>>>    #include "ecore_spq.h"
>>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>>>    							      p_aeu->bit_name,
>>>>    							      num);
>>>>    					else
>>>> -						OSAL_STRNCPY(bit_name,
>>>> -							     p_aeu->bit_name,
>>>> -							     30);
>>>> +						strlcpy(bit_name,
>>>> +							p_aeu->bit_name,
>>>> +							sizeof(bit_name));
>>>>
>>>>    					/* We now need to pass bitmask in its
>>>>    					 * correct position.
>>>
>>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
>>> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
>>>
>>> However, this modifies base driver code, so it is up to the maintainers to make
>> that decision.
>>> (CC'ing maintainers here).
>>
>> There's no value for any OSAL_* that simply defines itself to be the same as the
>> direct api, as does OSAL_STRNCPY.
>>
>> It's better to just remove any OSAL_* that calls straight through since all it does
>> is obfuscate what the code does, for no benefit.
> 
> I agree. Since this is modifying base driver code,
> the maintainers can decide what to do with this.
> 
>>
>>> Also, missing fixes line and CC stable.
>>>
>>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
>>
>> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
>> If so what's the benefit of that over using git if you ever want to know that?
>>
> 
> This is important mainly to see which releases needed this patch backported.
> I am replying to your patches with this info, to save you some time
> (I know you are feeling the pain of this overhead :P).

Yeah: I appreciate the help.  Some of the current rules make assumptions 
about burning time for no real benefit that assume the contributor has 
no choice but to comply.  But that is simply not true for all potential 
contributors.

I will integrate the comments tomorrow morning (ie, +12h) and push 
again.  I will look closer at the missing include thing, but since build 
stops, telling me it can't find the include file, and the patch fixes 
it, there are a limited amount of possible root causes.

-Andy

> 
>> -Andy
>>
>>> Cc: stable@dpdk.org
>>>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 2:39 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
> 
> 
> 
> On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andy Green [mailto:andy@warmcat.com]
> >> Sent: Friday, May 11, 2018 11:48 AM
> >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> >> dev@dpdk.org
> >> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil,
> >> Harish <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> >> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >>
> >>
> >> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >>>> Sent: Friday, May 11, 2018 2:46 AM
> >>>> To: dev@dpdk.org
> >>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >>>> constant and NUL
> >>>>
> >>>> Signed-off-by: Andy Green <andy@warmcat.com>
> >>>> ---
> >>>>    drivers/net/qede/base/ecore_int.c |    8 +++++---
> >>>>    1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/qede/base/ecore_int.c
> >>>> b/drivers/net/qede/base/ecore_int.c
> >>>> index f43781ba4..d9e22b5ed 100644
> >>>> --- a/drivers/net/qede/base/ecore_int.c
> >>>> +++ b/drivers/net/qede/base/ecore_int.c
> >>>> @@ -6,6 +6,8 @@
> >>>>     * See LICENSE.qede_pmd for copyright and licensing details.
> >>>>     */
> >>>>
> >>>> +#include <rte_string_fns.h>
> >>>> +
> >>>>    #include "bcm_osal.h"
> >>>>    #include "ecore.h"
> >>>>    #include "ecore_spq.h"
> >>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >>>>    							      p_aeu->bit_name,
> >>>>    							      num);
> >>>>    					else
> >>>> -						OSAL_STRNCPY(bit_name,
> >>>> -							     p_aeu->bit_name,
> >>>> -							     30);
> >>>> +						strlcpy(bit_name,
> >>>> +							p_aeu->bit_name,
> >>>> +							sizeof(bit_name));
> >>>>
> >>>>    					/* We now need to pass bitmask in its
> >>>>    					 * correct position.
> >>>
> >>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> >>> and modify the macro to use strlcpy, so we avoid further uses of that
> strlcpy.
> >>>
> >>> However, this modifies base driver code, so it is up to the
> >>> maintainers to make
> >> that decision.
> >>> (CC'ing maintainers here).
> >>
> >> There's no value for any OSAL_* that simply defines itself to be the
> >> same as the direct api, as does OSAL_STRNCPY.
> >>
> >> It's better to just remove any OSAL_* that calls straight through
> >> since all it does is obfuscate what the code does, for no benefit.
> >
> > I agree. Since this is modifying base driver code, the maintainers can
> > decide what to do with this.
> >
> >>
> >>> Also, missing fixes line and CC stable.
> >>>
> >>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting
> >>> string")
> >>
> >> The idea of this "Fixes" thing is to look up in git blame what is being edited is
> it?
> >> If so what's the benefit of that over using git if you ever want to know that?
> >>
> >
> > This is important mainly to see which releases needed this patch backported.
> > I am replying to your patches with this info, to save you some time (I
> > know you are feeling the pain of this overhead :P).
> 
> Yeah: I appreciate the help.  Some of the current rules make assumptions about
> burning time for no real benefit that assume the contributor has no choice but
> to comply.  But that is simply not true for all potential contributors.
> 
> I will integrate the comments tomorrow morning (ie, +12h) and push again.  I
> will look closer at the missing include thing, but since build stops, telling me it
> can't find the include file, and the patch fixes it, there are a limited amount of
> possible root causes.

Thanks for your time, Andy.

Pablo

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL

[Shaikh, Shahed]

> > >>
> > >> +#include <rte_string_fns.h>
> > >> +
> > >>   #include "bcm_osal.h"
> > >>   #include "ecore.h"
> > >>   #include "ecore_spq.h"
> > >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> > >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > >>   							      p_aeu->bit_name,
> > >>   							      num);
> > >>   					else
> > >> -						OSAL_STRNCPY(bit_name,
> > >> -							     p_aeu->bit_name,
> > >> -							     30);
> > >> +						strlcpy(bit_name,
> > >> +							p_aeu->bit_name,
> > >> +							sizeof(bit_name));
> > >>
> > >>   					/* We now need to pass bitmask in its
> > >>   					 * correct position.
> > >
> > > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> > > and modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> > >
> > > However, this modifies base driver code, so it is up to the
> > > maintainers to make
> > that decision.
> > > (CC'ing maintainers here).
> >
> > There's no value for any OSAL_* that simply defines itself to be the
> > same as the direct api, as does OSAL_STRNCPY.
> >
> > It's better to just remove any OSAL_* that calls straight through
> > since all it does is obfuscate what the code does, for no benefit.
> 
> I agree. Since this is modifying base driver code, the maintainers can decide
> what to do with this.

Hi,

For this series, you can continue with s/OSAL_STRNCPY/strlcpy/ for this instance.
I will send a patch to cleanup OSAL_* once your series gets applied.

Thanks,
Shahed

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 10/18] net/qede: solve broken strncpy

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 10/18] net/qede: solve broken strncpy
> 
> /home/agreen/projects/dpdk/drivers/net/qede/qede_main.c: In function
> ‘qed_slowpath_start’:
> /home/agreen/projects/dpdk/drivers/net/qede/qede_main.c:307:3:
> error: ‘strncpy’ output may be truncated copying 12 bytes from a string of length
> 127 [-Werror=stringop-truncation]
>    strncpy((char *)drv_version.name, (const char *)params->name,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     MCP_DRV_VER_STR_SIZE - 4);
>     ~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/qede/qede_main.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
> index 2333ca073..fcfc32d0d 100644
> --- a/drivers/net/qede/qede_main.c
> +++ b/drivers/net/qede/qede_main.c
> @@ -9,6 +9,7 @@
>  #include <limits.h>
>  #include <time.h>
>  #include <rte_alarm.h>
> +#include <rte_string_fns.h>
> 
>  #include "qede_ethdev.h"
> 
> @@ -303,9 +304,9 @@ static int qed_slowpath_start(struct ecore_dev *edev,
>  		drv_version.version = (params->drv_major << 24) |
>  		    (params->drv_minor << 16) |
>  		    (params->drv_rev << 8) | (params->drv_eng);
> -		/* TBD: strlcpy() */
> -		strncpy((char *)drv_version.name, (const char *)params->name,
> -			MCP_DRV_VER_STR_SIZE - 4);
> +		strlcpy((char *)drv_version.name, (const char *)params->name,
> +			sizeof(drv_version.name));
> +		drv_version.name[sizeof(drv_version.name) - 1] = '\0';

Strlcpy already terminates the buffer with NULL character, so this last line is not needed.

Also, missing fix line and CC stable

Fixes: 86a2265e59d7 ("qede: add SRIOV support")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/sfc/sfc_ethdev.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
> e42d55350..ef5e9ecb2 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -13,6 +13,7 @@
>  #include <rte_pci.h>
>  #include <rte_bus_pci.h>
>  #include <rte_errno.h>
> +#include <rte_string_fns.h>
> 
>  #include "efx.h"
> 
> @@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
>  		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
>  			char *name = xstats_names[nb_written++].name;
> 
> -			strncpy(name, efx_mac_stat_name(sa->nic, i),
> +			strlcpy(name, efx_mac_stat_name(sa->nic, i),
>  				sizeof(xstats_names[0].name));

Shouldn't this be "sizeof(name)"? Although probably it is the same.

Missing fixes line and CC stable.

Fixes: 73280c1e4ff2 ("net/sfc: support xstats retrieval by ID")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length

[Andy Green]

On 05/11/2018 06:51 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 11/18] net/sfc: correct strncpy length
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/sfc/sfc_ethdev.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
>> e42d55350..ef5e9ecb2 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -13,6 +13,7 @@
>>   #include <rte_pci.h>
>>   #include <rte_bus_pci.h>
>>   #include <rte_errno.h>
>> +#include <rte_string_fns.h>
>>
>>   #include "efx.h"
>>
>> @@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
>>   		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
>>   			char *name = xstats_names[nb_written++].name;
>>
>> -			strncpy(name, efx_mac_stat_name(sa->nic, i),
>> +			strlcpy(name, efx_mac_stat_name(sa->nic, i),
>>   				sizeof(xstats_names[0].name));
> 
> Shouldn't this be "sizeof(name)"? Although probably it is the same.

name is just a pointer, so sizeof that is 4 or 8.

-Andy

> Missing fixes line and CC stable.
> 
> Fixes: 73280c1e4ff2 ("net/sfc: support xstats retrieval by ID")
> Cc: stable@dpdk.org
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  drivers/net/sfc/sfc_ethdev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
> ef5e9ecb2..a8c0f8e19 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -664,7 +664,7 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>  	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>  		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>  			if (xstats_names != NULL && nstats < xstats_count)
> -				strncpy(xstats_names[nstats].name,
> +				strlcpy(xstats_names[nstats].name,
>  					efx_mac_stat_name(sa->nic, i),
>  					sizeof(xstats_names[0].name));
>  			nstats++;

I'd  say this patch could be squashed with the previous one, as they are solving the same issue
in the same file.
It also needs an extra fixes line (so the final patch would have two fixes lines) and CC stable too.

Fixes: 7b9891769f4b ("net/sfc: support extended statistics")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL

[Andy Green]

On 05/11/2018 06:55 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 12/18] net/sfc: solve strncpy size and NUL
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   drivers/net/sfc/sfc_ethdev.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
>> ef5e9ecb2..a8c0f8e19 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -664,7 +664,7 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>>   	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>>   		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>>   			if (xstats_names != NULL && nstats < xstats_count)
>> -				strncpy(xstats_names[nstats].name,
>> +				strlcpy(xstats_names[nstats].name,
>>   					efx_mac_stat_name(sa->nic, i),
>>   					sizeof(xstats_names[0].name));
>>   			nstats++;
> 
> I'd  say this patch could be squashed with the previous one, as they are solving the same issue
> in the same file.
> It also needs an extra fixes line (so the final patch would have two fixes lines) and CC stable too.

OK it's adapted accordingly.

-Andy

> Fixes: 7b9891769f4b ("net/sfc: support extended statistics")
> Cc: stable@dpdk.org
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 14/18] net/vdev_netvsc: 3 x strncpy misuse

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 14/18] net/vdev_netvsc: 3 x strncpy misuse
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable:

Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")
Cc: stable@dpdk.org

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug
> 
> /home/agreen/projects/dpdk/app/proc-info/main.c: In function
> ‘nic_xstats_display’:
> /home/agreen/projects/dpdk/app/proc-info/main.c:495:45:
> error: ‘%s’ directive writing up to 255 bytes into a region of size between 165
> and 232 [-Werror=format-overflow=]
>     sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>                                              ^~
>      PRIu64"\n", host_id, port_id, counter_type,
>                                    ~~~~~~~~~~~~
> /home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note:
> ‘sprintf’ output between 31 and 435 bytes into a destination of size 256
>     sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      PRIu64"\n", host_id, port_id, counter_type,
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      xstats_names[i].name, values[i]);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  app/proc-info/main.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c index
> 539e13243..df46c235e 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id)
>  		if (enable_collectd_format) {
>  			char counter_type[MAX_STRING_LEN];
>  			char buf[MAX_STRING_LEN];
> +			size_t n;
> 
>  			collectd_resolve_cnt_type(counter_type,
>  						  sizeof(counter_type),
>  						  xstats_names[i].name);
> -			sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
> +			n = snprintf(buf, MAX_STRING_LEN,
> +				"PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>  				PRIu64"\n", host_id, port_id, counter_type,
>  				xstats_names[i].name, values[i]);
> -			ret = write(stdout_fd, buf, strlen(buf));
> +			buf[sizeof(buf) - 1] = '\0';

No need to NULL terminate, since snprintf already does it.

> +			if (n > sizeof(buf) - 1)
> +				n = sizeof(buf) - 1;

If (n > sizeof(buf) -1 ), it means that there wasn't enough space to write all the data,
So shouldn't we return an error here?

> +			ret = write(stdout_fd, buf, n);
>  			if (ret < 0)
>  				goto err;
>  		} else {

Missing fixes line and CC stable.

Fixes: 2deb6b5246d7 ("app/procinfo: add collectd format and host id")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug

[Andy Green]

On 05/11/2018 08:26 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:47 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug
>>
>> /home/agreen/projects/dpdk/app/proc-info/main.c: In function
>> ‘nic_xstats_display’:
>> /home/agreen/projects/dpdk/app/proc-info/main.c:495:45:
>> error: ‘%s’ directive writing up to 255 bytes into a region of size between 165
>> and 232 [-Werror=format-overflow=]
>>      sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>>                                               ^~
>>       PRIu64"\n", host_id, port_id, counter_type,
>>                                     ~~~~~~~~~~~~
>> /home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note:
>> ‘sprintf’ output between 31 and 435 bytes into a destination of size 256
>>      sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       PRIu64"\n", host_id, port_id, counter_type,
>>       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>       xstats_names[i].name, values[i]);
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   app/proc-info/main.c |    9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/proc-info/main.c b/app/proc-info/main.c index
>> 539e13243..df46c235e 100644
>> --- a/app/proc-info/main.c
>> +++ b/app/proc-info/main.c
>> @@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id)
>>   		if (enable_collectd_format) {
>>   			char counter_type[MAX_STRING_LEN];
>>   			char buf[MAX_STRING_LEN];
>> +			size_t n;
>>
>>   			collectd_resolve_cnt_type(counter_type,
>>   						  sizeof(counter_type),
>>   						  xstats_names[i].name);
>> -			sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>> +			n = snprintf(buf, MAX_STRING_LEN,
>> +				"PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>>   				PRIu64"\n", host_id, port_id, counter_type,
>>   				xstats_names[i].name, values[i]);
>> -			ret = write(stdout_fd, buf, strlen(buf));
>> +			buf[sizeof(buf) - 1] = '\0';
> 
> No need to NULL terminate, since snprintf already does it.

OK.

>> +			if (n > sizeof(buf) - 1)
>> +				n = sizeof(buf) - 1;
> 
> If (n > sizeof(buf) -1 ), it means that there wasn't enough space to write all the data,
> So shouldn't we return an error here?

It's just logging stuff, policy about truncated overlength logs could go 
either way.

Prior to this patch its policy was to corrupt your stack if overlength 
;-) so I think it's moving it on in the right direction merely 
truncating it.

-Andy

>> +			ret = write(stdout_fd, buf, n);
>>   			if (ret < 0)
>>   				goto err;
>>   		} else {
> 
> Missing fixes line and CC stable.
> 
> Fixes: 2deb6b5246d7 ("app/procinfo: add collectd format and host id")
> Cc: stable@dpdk.org
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 17/18] app/test-bbdev: strcpy ok for allocated string

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 17/18] app/test-bbdev: strcpy ok for allocated
> string
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  app/test-bbdev/test_bbdev_vector.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-
> bbdev/test_bbdev_vector.c
> index a37e35f4d..c574f2135 100644
> --- a/app/test-bbdev/test_bbdev_vector.c
> +++ b/app/test-bbdev/test_bbdev_vector.c
> @@ -892,7 +892,7 @@ test_bbdev_vector_read(const char *filename,
>  		}
> 
>  		memset(entry, 0, strlen(line) + 1);
> -		strncpy(entry, line, strlen(line));
> +		strcpy(entry, line);

Better to use strlcpy(entry,line,sizeof(entry)) and remove the memset call.

Missing fixes line and CC stable:

Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated string

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated
> string
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  app/test-bbdev/test_bbdev_vector.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-
> bbdev/test_bbdev_vector.c
> index c574f2135..4a3ddcffe 100644
> --- a/app/test-bbdev/test_bbdev_vector.c
> +++ b/app/test-bbdev/test_bbdev_vector.c
> @@ -914,7 +914,8 @@ test_bbdev_vector_read(const char *filename,
>  				}
> 
>  				entry = entry_extended;
> -				strncat(entry, line, strlen(line));
> +				/* entry has been allocated accordingly */
> +				strcpy(&entry[strlen(entry)], line);

If memset is removed in the previous patch, then we'll need to use strlcpy
here, to ensure NULL termination.

Missing fixes line and CC stable:

Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
Cc: stable@dpdk.org

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated string

[Andy Green]

On 05/11/2018 09:02 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:47 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 18/18] app/test-bbdev: strcpy ok for allocated
>> string
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   app/test-bbdev/test_bbdev_vector.c |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-
>> bbdev/test_bbdev_vector.c
>> index c574f2135..4a3ddcffe 100644
>> --- a/app/test-bbdev/test_bbdev_vector.c
>> +++ b/app/test-bbdev/test_bbdev_vector.c
>> @@ -914,7 +914,8 @@ test_bbdev_vector_read(const char *filename,
>>   				}
>>
>>   				entry = entry_extended;
>> -				strncat(entry, line, strlen(line));
>> +				/* entry has been allocated accordingly */
>> +				strcpy(&entry[strlen(entry)], line);
> 
> If memset is removed in the previous patch, then we'll need to use strlcpy
> here, to ensure NULL termination.

No... the destination has been allocated dynamically so there is no case 
where the source length can exceed the destination.

I think for that reason it's OK to remove the memset() here, because for 
the same reason it will always exactly fill the dynamically sized string 
anyway.

-Andy

> Missing fixes line and CC stable:
> 
> Fixes: f714a18885a6 ("app/testbbdev: add test application for bbdev")
> Cc: stable@dpdk.org
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4 13/18] net/vdev_netvsc: readlink inputs cannot be aliased

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 13/18] net/vdev_netvsc: readlink inputs cannot
> be aliased
> 
> /home/agreen/projects/dpdk/drivers/net/vdev_netvsc/
> vdev_netvsc.c:335:2:error: passing argument 2 to restrict- qualified parameter
> aliases with argument 1 [-Werror=restrict]
>   ret = readlink(buf, buf, size);
>   ^~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Missing fixes line and CC stable:

Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>