[ovs-dev,2/8] datapath: introduce nf_conntrack_helper_put function

Upstream commit:
    commit d91fc59cd77c719f33eda65c194ad8f95a055190
    Author: Liping Zhang <zlpnobody@gmail.com>
    Date:   Sun May 7 22:01:55 2017 +0800

    netfilter: introduce nf_conntrack_helper_put helper function

    And convert module_put invocation to nf_conntrack_helper_put, this is
    prepared for the followup patch, which will add a refcnt for cthelper,
    so we can reject the deleting request when cthelper is in use.

    Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied with additional use of HAVE_NF_CONNTRACK_HELPER_PUT compatibility
flag defined in acinclude.m4.

Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 datapath/conntrack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

On 19 July 2017 at 13:57, Greg Rose <gvrose8192@gmail.com> wrote:
> Upstream commit:
>     commit d91fc59cd77c719f33eda65c194ad8f95a055190
>     Author: Liping Zhang <zlpnobody@gmail.com>
>     Date:   Sun May 7 22:01:55 2017 +0800
>
>     netfilter: introduce nf_conntrack_helper_put helper function
>
>     And convert module_put invocation to nf_conntrack_helper_put, this is
>     prepared for the followup patch, which will add a refcnt for cthelper,
>     so we can reject the deleting request when cthelper is in use.
>
>     Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>     Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
> Applied with additional use of HAVE_NF_CONNTRACK_HELPER_PUT compatibility
> flag defined in acinclude.m4.
>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> ---
>  datapath/conntrack.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index bf28fc0..cce2642 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1164,7 +1164,11 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
>
>         help = nf_ct_helper_ext_add(info->ct, helper, GFP_KERNEL);
>         if (!help) {
> +#ifdef HAVE_NF_CONTRACK_HELPER_PUT
> +               nf_contrack_helper_put(helper);
> +#else
>                 module_put(helper->me);
> +#endif
>                 return -ENOMEM;
>         }
>
> @@ -1634,7 +1638,11 @@ void ovs_ct_free_action(const struct nlattr *a)
>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>  {
>         if (ct_info->helper)
> +#ifdef HAVE_NF_CONNTRACK_HELPER_PUT
> +               nf_conntrack_helper_put(ct_info->helper);
> +#else
>                 module_put(ct_info->helper->me);
> +#endif
>         if (ct_info->ct)
>                 nf_ct_tmpl_free(ct_info->ct);
>  }

Ideally the datapath/*.[ch] code is as close to upstream as possible,
then the datapath/linux/compat/* takes care of making this happen.

To achieve this, in this kind of case, what we will often do is to
introduce a new function like this in the
datapath/linux/compat/net/netfilter/nf_conntrack_helper.h:

#ifndef HAVE_NF_CONNTRACK_HELPER_PUT
static inline void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
{
    module_put(helper->me);
}
#endif

Then, in datapath/conntrack.c we don't need any #ifdef; it can just
directly call the new nf_conntrack_helper_put() function.

Thanks.

Right, that's a cleaner solution.  Thanks for pointing it out.  I
think there are a few others like that in the series then.

- Greg

On Fri, Jul 21, 2017 at 11:42 AM, Joe Stringer <joe@ovn.org> wrote:
> On 19 July 2017 at 13:57, Greg Rose <gvrose8192@gmail.com> wrote:
>> Upstream commit:
>>     commit d91fc59cd77c719f33eda65c194ad8f95a055190
>>     Author: Liping Zhang <zlpnobody@gmail.com>
>>     Date:   Sun May 7 22:01:55 2017 +0800
>>
>>     netfilter: introduce nf_conntrack_helper_put helper function
>>
>>     And convert module_put invocation to nf_conntrack_helper_put, this is
>>     prepared for the followup patch, which will add a refcnt for cthelper,
>>     so we can reject the deleting request when cthelper is in use.
>>
>>     Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>>     Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>
>> Applied with additional use of HAVE_NF_CONNTRACK_HELPER_PUT compatibility
>> flag defined in acinclude.m4.
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> ---
>>  datapath/conntrack.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
>> index bf28fc0..cce2642 100644
>> --- a/datapath/conntrack.c
>> +++ b/datapath/conntrack.c
>> @@ -1164,7 +1164,11 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
>>
>>         help = nf_ct_helper_ext_add(info->ct, helper, GFP_KERNEL);
>>         if (!help) {
>> +#ifdef HAVE_NF_CONTRACK_HELPER_PUT
>> +               nf_contrack_helper_put(helper);
>> +#else
>>                 module_put(helper->me);
>> +#endif
>>                 return -ENOMEM;
>>         }
>>
>> @@ -1634,7 +1638,11 @@ void ovs_ct_free_action(const struct nlattr *a)
>>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>>  {
>>         if (ct_info->helper)
>> +#ifdef HAVE_NF_CONNTRACK_HELPER_PUT
>> +               nf_conntrack_helper_put(ct_info->helper);
>> +#else
>>                 module_put(ct_info->helper->me);
>> +#endif
>>         if (ct_info->ct)
>>                 nf_ct_tmpl_free(ct_info->ct);
>>  }
>
> Ideally the datapath/*.[ch] code is as close to upstream as possible,
> then the datapath/linux/compat/* takes care of making this happen.
>
> To achieve this, in this kind of case, what we will often do is to
> introduce a new function like this in the
> datapath/linux/compat/net/netfilter/nf_conntrack_helper.h:
>
> #ifndef HAVE_NF_CONNTRACK_HELPER_PUT
> static inline void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
> {
>     module_put(helper->me);
> }
> #endif
>
> Then, in datapath/conntrack.c we don't need any #ifdef; it can just
> directly call the new nf_conntrack_helper_put() function.
>
> Thanks.

On 21 July 2017 at 11:47, Greg Rose <gvrose8192@gmail.com> wrote:
> Right, that's a cleaner solution.  Thanks for pointing it out.  I
> think there are a few others like that in the series then.

OK, I'll keep an eye out for those.