netdev - Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Feb 2023 15:15:20 +0800
From:   Gavin Li <gavinl@...dia.com>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, roopa@...dia.com,
        eng.alaamohamedsoliman.am@...il.com, bigeasy@...utronix.de,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        gavi@...dia.com, roid@...dia.com, maord@...dia.com,
        saeedm@...dia.com
Subject: Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr


On 2/20/2023 2:40 PM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
>> On 2/20/2023 4:32 AM, Simon Horman wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
>>>> vxlan_build_gbp_hdr will be used by other modules to build gbp option in
>>>> vxlan header according to gbp flags.
>>>>
>>>> Signed-off-by: Gavin Li <gavinl@...dia.com>
>>>> Reviewed-by: Gavi Teitz <gavi@...dia.com>
>>>> Reviewed-by: Roi Dayan <roid@...dia.com>
>>>> Reviewed-by: Maor Dickman <maord@...dia.com>
>>>> Acked-by: Saeed Mahameed <saeedm@...dia.com>
>>> I do wonder if this needs to be a static inline function.
>>> But nonetheless,
>> Will get "unused-function" from gcc without "inline"
>>
>> ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
>> used [-Wunused-function]
>>   static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
>> vxlan_metadata *md)
> Right. But what I was really wondering is if the definition
> of the function could stay in drivers/net/vxlan/vxlan_core.c,
> without being static. And have a declaration in include/net/vxlan.h

Tried that the first time the function was called by driver code. It 
would introduce dependency in linking between the driver and the kernel 
module.

Do you think it's OK to have such dependency?

>
>>> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>>>
>>>> ---
>>>>    drivers/net/vxlan/vxlan_core.c | 19 -------------------
>>>>    include/net/vxlan.h            | 19 +++++++++++++++++++
>>>>    2 files changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>>>> index 86967277ab97..13faab36b3e1 100644
>>>> --- a/drivers/net/vxlan/vxlan_core.c
>>>> +++ b/drivers/net/vxlan/vxlan_core.c
>>>> @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
>>>>         return false;
>>>>    }
>>>>
>>>> -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
>>>> -{
>>>> -     struct vxlanhdr_gbp *gbp;
>>>> -
>>>> -     if (!md->gbp)
>>>> -             return;
>>>> -
>>>> -     gbp = (struct vxlanhdr_gbp *)vxh;
>>>> -     vxh->vx_flags |= VXLAN_HF_GBP;
>>>> -
>>>> -     if (md->gbp & VXLAN_GBP_DONT_LEARN)
>>>> -             gbp->dont_learn = 1;
>>>> -
>>>> -     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>>>> -             gbp->policy_applied = 1;
>>>> -
>>>> -     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>>>> -}
>>>> -
>>>>    static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
>>>>    {
>>>>         struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
>>>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>>>> index bca5b01af247..b6d419fa7ab1 100644
>>>> --- a/include/net/vxlan.h
>>>> +++ b/include/net/vxlan.h
>>>> @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
>>>>         return true;
>>>>    }
>>>>
>>>> +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
>>>> +{
>>>> +     struct vxlanhdr_gbp *gbp;
>>>> +
>>>> +     if (!md->gbp)
>>>> +             return;
>>>> +
>>>> +     gbp = (struct vxlanhdr_gbp *)vxh;
>>>> +     vxh->vx_flags |= VXLAN_HF_GBP;
>>>> +
>>>> +     if (md->gbp & VXLAN_GBP_DONT_LEARN)
>>>> +             gbp->dont_learn = 1;
>>>> +
>>>> +     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
>>>> +             gbp->policy_applied = 1;
>>>> +
>>>> +     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
>>>> +}
>>>> +
>>>>    #endif
>>>> --
>>>> 2.31.1
>>>>