Re: [PATCH nf-next] netfilter: connlabels: Export setting connlabel length

[Joe Stringer <joestringer@nicira.com>]

On 31 July 2015 at 17:40, Florian Westphal <fw@strlen.de> wrote:
> Joe Stringer <joestringer@nicira.com> wrote:
>> diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
>> index bb53f12..00df4e71 100644
>> --- a/net/netfilter/nf_conntrack_labels.c
>> +++ b/net/netfilter/nf_conntrack_labels.c
>> @@ -91,6 +91,30 @@ int nf_connlabels_replace(struct nf_conn *ct,
>>  EXPORT_SYMBOL_GPL(nf_connlabels_replace);
>>  #endif
>>
>> +int nf_connlabels_get(struct net *net, unsigned int n_bits)
>> +{
>> +     size_t words;
>> +
>> +     if (n_bits > XT_CONNLABEL_MAXBIT + 1)
>
> Perhaps use
>
>   if (n_bits >= (NF_CT_LABELS_MAX_SIZE * BITS_PER_BYTE)))
>
> To avoid the XT_CONNLABEL_MAXBIT in the nf_* part.
>
>> +             return -ERANGE;
>> +
>> +     net->ct.labels_used++;
>> +     words = BITS_TO_LONGS(n_bits);
>> +     if (words > net->ct.label_words)
>> +             net->ct.label_words = words;
>> +
>> +     return 0;
>> +}
>
> I think we should add a lock here, currently this is protected by the
> xtables mutex -- I'd suggest to just add a spinlock for this.
>
>>               return ret;
>>       }
>>
>> -     par->net->ct.labels_used++;
>> -     words = BITS_TO_LONGS(info->bit+1);
>> -     if (words > par->net->ct.label_words)
>> -             par->net->ct.label_words = words;
>> -
>> -     return ret;
>> +     return nf_connlabels_get(par->net, info->bit + 1);
>
> This can leak the refcnt on l3_proto_module we obtained earlier.
>
> Other than that, this looks good.
>
> Thanks,
> Florian

Thanks for the review, I'll fix these up and send a v2 soon.