net/mlx5: fix eCPRI previous layer checking
Checks
Commit Message
Based on the specification, eCPRI can only follow ETH (VLAN) layer
or UDP layer. When creating a flow with eCPRI item, this should be
checked and invalid layout of the layers should be rejected.
Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI header")
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
Comments
Hi,
> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Tuesday, November 3, 2020 7:43 AM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>
> Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; stable@dpdk.org
> Subject: [PATCH] net/mlx5: fix eCPRI previous layer checking
>
> Based on the specification, eCPRI can only follow ETH (VLAN) layer
> or UDP layer. When creating a flow with eCPRI item, this should be
> checked and invalid layout of the layers should be rejected.
>
> Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI header")
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
Patch applied to next-net-mlx,
Kindest regards
Raslan Darawsheh
On 11/3/2020 5:42 AM, Bing Zhao wrote:
> Based on the specification, eCPRI can only follow ETH (VLAN) layer
> or UDP layer. When creating a flow with eCPRI item, this should be
> checked and invalid layout of the layers should be rejected.
>
> Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI header")
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index a6e60af..11dba3b 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2896,17 +2896,23 @@ struct mlx5_flow_tunnel_info {
> MLX5_FLOW_LAYER_OUTER_VLAN);
> struct rte_flow_item_ecpri mask_lo;
>
> + if (!(last_item & outer_l2_vlan) &&
> + last_item != MLX5_FLOW_LAYER_OUTER_L4_UDP)
> + return rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM, item,
> + "eCPRI can only follow L2/VLAN layer"
> + " or UDP layer.");
> if ((last_item & outer_l2_vlan) && ether_type &&
> ether_type != RTE_ETHER_TYPE_ECPRI)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM, item,
> - "eCPRI cannot follow L2/VLAN layer "
> - "which ether type is not 0xAEFE.");
> + "eCPRI cannot follow L2/VLAN layer"
> + " which ether type is not 0xAEFE.");
> if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
> return rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM, item,
> - "eCPRI with tunnel is not supported "
> - "right now.");
> + "eCPRI with tunnel is not supported"
> + " right now.");
Why these changes done, it only moves space from end of first line to beginning
of the second line?
Overall I think no need to break the log strings, keeping them intact helps
users search the error message in the code.
I assume the break is because of the 80 chars limit but for log strings we don't
have that limit, unless it is too long (lets say 120 chars as thumb of rule,
there is no official convention) I think better to not break.
What do you think remove the whitespace changes out of this commit and make
another patch to merge the log strings?
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, November 6, 2020 7:35 PM
> To: Bing Zhao <bingz@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix eCPRI previous
> layer checking
>
> External email: Use caution opening links or attachments
>
>
> On 11/3/2020 5:42 AM, Bing Zhao wrote:
> > Based on the specification, eCPRI can only follow ETH (VLAN) layer
> or
> > UDP layer. When creating a flow with eCPRI item, this should be
> > checked and invalid layout of the layers should be rejected.
> >
> > Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI
> header")
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> > ---
> > drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
> > 1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index a6e60af..11dba3b 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2896,17 +2896,23 @@ struct mlx5_flow_tunnel_info {
> > MLX5_FLOW_LAYER_OUTER_VLAN);
> > struct rte_flow_item_ecpri mask_lo;
> >
> > + if (!(last_item & outer_l2_vlan) &&
> > + last_item != MLX5_FLOW_LAYER_OUTER_L4_UDP)
> > + return rte_flow_error_set(error, EINVAL,
> > + RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > + "eCPRI can only follow
> L2/VLAN layer"
> > + " or UDP layer.");
> > if ((last_item & outer_l2_vlan) && ether_type &&
> > ether_type != RTE_ETHER_TYPE_ECPRI)
> > return rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > - "eCPRI cannot follow
> L2/VLAN layer "
> > - "which ether type is not
> 0xAEFE.");
> > + "eCPRI cannot follow
> L2/VLAN layer"
> > + " which ether type is not
> > + 0xAEFE.");
> > if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
> > return rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > - "eCPRI with tunnel is not
> supported "
> > - "right now.");
> > + "eCPRI with tunnel is not
> supported"
> > + " right now.");
>
> Why these changes done, it only moves space from end of first line
> to beginning of the second line?
Yes, because when I am doing the fix. I found this log part is different from others in the same file and just want to be consistent.
>
> Overall I think no need to break the log strings, keeping them
> intact helps users search the error message in the code.
> I assume the break is because of the 80 chars limit but for log
> strings we don't have that limit, unless it is too long (lets say
> 120 chars as thumb of rule, there is no official convention) I think
> better to not break.
Good point, in the past when I was searching some logs and I failed due to the long log line break.
>
> What do you think remove the whitespace changes out of this commit
> and make another patch to merge the log strings?
Yes I can and will send v2 of this.
Or should I keep the log in a single line @Slava Ovsiienko, what do you think? Any comments?
I remember in the past, my "checkpatch.pl" will report warning against this. Could we ignore this?
BR. Bing
On 11/6/2020 2:20 PM, Bing Zhao wrote:
> Hi Ferruh,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Friday, November 6, 2020 7:35 PM
>> To: Bing Zhao <bingz@nvidia.com>; Slava Ovsiienko
>> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>
>> Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; Raslan Darawsheh
>> <rasland@nvidia.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix eCPRI previous
>> layer checking
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 11/3/2020 5:42 AM, Bing Zhao wrote:
>>> Based on the specification, eCPRI can only follow ETH (VLAN) layer
>> or
>>> UDP layer. When creating a flow with eCPRI item, this should be
>>> checked and invalid layout of the layers should be rejected.
>>>
>>> Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI
>> header")
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Bing Zhao <bingz@nvidia.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>> ---
>>> drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
>>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/mlx5/mlx5_flow.c
>>> b/drivers/net/mlx5/mlx5_flow.c index a6e60af..11dba3b 100644
>>> --- a/drivers/net/mlx5/mlx5_flow.c
>>> +++ b/drivers/net/mlx5/mlx5_flow.c
>>> @@ -2896,17 +2896,23 @@ struct mlx5_flow_tunnel_info {
>>> MLX5_FLOW_LAYER_OUTER_VLAN);
>>> struct rte_flow_item_ecpri mask_lo;
>>>
>>> + if (!(last_item & outer_l2_vlan) &&
>>> + last_item != MLX5_FLOW_LAYER_OUTER_L4_UDP)
>>> + return rte_flow_error_set(error, EINVAL,
>>> + RTE_FLOW_ERROR_TYPE_ITEM,
>> item,
>>> + "eCPRI can only follow
>> L2/VLAN layer"
>>> + " or UDP layer.");
>>> if ((last_item & outer_l2_vlan) && ether_type &&
>>> ether_type != RTE_ETHER_TYPE_ECPRI)
>>> return rte_flow_error_set(error, EINVAL,
>>> RTE_FLOW_ERROR_TYPE_ITEM,
>> item,
>>> - "eCPRI cannot follow
>> L2/VLAN layer "
>>> - "which ether type is not
>> 0xAEFE.");
>>> + "eCPRI cannot follow
>> L2/VLAN layer"
>>> + " which ether type is not
>>> + 0xAEFE.");
>>> if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
>>> return rte_flow_error_set(error, EINVAL,
>>> RTE_FLOW_ERROR_TYPE_ITEM,
>> item,
>>> - "eCPRI with tunnel is not
>> supported "
>>> - "right now.");
>>> + "eCPRI with tunnel is not
>> supported"
>>> + " right now.");
>>
>> Why these changes done, it only moves space from end of first line
>> to beginning of the second line?
>
> Yes, because when I am doing the fix. I found this log part is different from others in the same file and just want to be consistent.
>
>>
>> Overall I think no need to break the log strings, keeping them
>> intact helps users search the error message in the code.
>> I assume the break is because of the 80 chars limit but for log
>> strings we don't have that limit, unless it is too long (lets say
>> 120 chars as thumb of rule, there is no official convention) I think
>> better to not break.
>
> Good point, in the past when I was searching some logs and I failed due to the long log line break.
>
>>
>> What do you think remove the whitespace changes out of this commit
>> and make another patch to merge the log strings?
>
> Yes I can and will send v2 of this.
> Or should I keep the log in a single line @Slava Ovsiienko, what do you think? Any comments?
> I remember in the past, my "checkpatch.pl" will report warning against this. Could we ignore this?
>
As far as I know checkpatch is not complaining for the long lines of the string,
even it does I am OK to ignore it.
@@ -2896,17 +2896,23 @@ struct mlx5_flow_tunnel_info {
MLX5_FLOW_LAYER_OUTER_VLAN);
struct rte_flow_item_ecpri mask_lo;
+ if (!(last_item & outer_l2_vlan) &&
+ last_item != MLX5_FLOW_LAYER_OUTER_L4_UDP)
+ return rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_ITEM, item,
+ "eCPRI can only follow L2/VLAN layer"
+ " or UDP layer.");
if ((last_item & outer_l2_vlan) && ether_type &&
ether_type != RTE_ETHER_TYPE_ECPRI)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM, item,
- "eCPRI cannot follow L2/VLAN layer "
- "which ether type is not 0xAEFE.");
+ "eCPRI cannot follow L2/VLAN layer"
+ " which ether type is not 0xAEFE.");
if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM, item,
- "eCPRI with tunnel is not supported "
- "right now.");
+ "eCPRI with tunnel is not supported"
+ " right now.");
if (item_flags & MLX5_FLOW_LAYER_OUTER_L3)
return rte_flow_error_set(error, ENOTSUP,
RTE_FLOW_ERROR_TYPE_ITEM, item,
@@ -2914,13 +2920,14 @@ struct mlx5_flow_tunnel_info {
else if (item_flags & MLX5_FLOW_LAYER_OUTER_L4_TCP)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM, item,
- "eCPRI cannot follow a TCP layer.");
+ "eCPRI cannot coexist with a"
+ " TCP layer.");
/* In specification, eCPRI could be over UDP layer. */
else if (item_flags & MLX5_FLOW_LAYER_OUTER_L4_UDP)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM, item,
- "eCPRI over UDP layer is not yet "
- "supported right now.");
+ "eCPRI over UDP layer is not yet"
+ " supported right now.");
/* Mask for type field in common header could be zero. */
if (!mask)
mask = &rte_flow_item_ecpri_mask;
@@ -2929,13 +2936,13 @@ struct mlx5_flow_tunnel_info {
if (mask_lo.hdr.common.type != 0 && mask_lo.hdr.common.type != 0xff)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM_MASK, mask,
- "partial mask is not supported "
- "for protocol");
+ "partial mask is not supported"
+ " for protocol");
else if (mask_lo.hdr.common.type == 0 && mask->hdr.dummy[0] != 0)
return rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM_MASK, mask,
- "message header mask must be after "
- "a type mask");
+ "message header mask must be after"
+ " a type mask");
return mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
acc_mask ? (const uint8_t *)acc_mask
: (const uint8_t *)&nic_mask,