Pablo,
Were you waiting for a v4 or are you ready to take this as-is?
Jarno
> On Apr 19, 2017, at 12:56 PM, Jarno Rajahalme <jarno@xxxxxxx> wrote:
>
>>
>> On Apr 19, 2017, at 3:30 AM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>>
>> On Tue, Apr 18, 2017 at 12:24:28PM -0700, Joe Stringer wrote:
>>> On 18 April 2017 at 11:33, Jarno Rajahalme <jarno@xxxxxxx> wrote:
>>>>
>>>>> On Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@xxxxxxx> wrote:
>>>>>
>>>>> On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@xxxxxxx> wrote:
>>>>>> Conntrack helpers do not check for a potentially clashing conntrack
>>>>>> entry when creating a new expectation. Also, nf_conntrack_in() will
>>>>>> check expectations (via init_conntrack()) only if a conntrack entry
>>>>>> can not be found. The expectation for a packet which also matches an
>>>>>> existing conntrack entry will not be removed by conntrack, and is
>>>>>> currently handled inconsistently by OVS, as OVS expects the
>>>>>> expectation to be removed when the connection tracking entry matching
>>>>>> that expectation is confirmed.
>>>>>>
>>>>>> It should be noted that normally an IP stack would not allow reuse of
>>>>>> a 5-tuple of an old (possibly lingering) connection for a new data
>>>>>> connection, so this is somewhat unlikely corner case. However, it is
>>>>>> possible that a misbehaving source could cause conntrack entries be
>>>>>> created that could then interfere with new related connections.
>>>>>>
>>>>>> Fix this in the OVS module by deleting the clashing conntrack entry
>>>>>> after an expectation has been matched. This causes the following
>>>>>> nf_conntrack_in() call also find the expectation and remove it when
>>>>>> creating the new conntrack entry, as well as the forthcoming reply
>>>>>> direction packets to match the new related connection instead of the
>>>>>> old clashing conntrack entry.
>>>>>>
>>>>>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
>>>>>> Reported-by: Yang Song <yangsong@xxxxxxxxxx>
>>>>>> Signed-off-by: Jarno Rajahalme <jarno@xxxxxxx>
>>>>>> ---
>>>>>
>>>>> Hi Jarno,
>>>>>
>>>>>> v3: Removed unnecessary if statement.
>>>>>> v2: Fixed commit title.
>>>>>>
>>>>>> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++-
>>>>>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>>>>> index 7b2c2fc..d796ae7 100644
>>>>>> --- a/net/openvswitch/conntrack.c
>>>>>> +++ b/net/openvswitch/conntrack.c
>>>>>> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone,
>>>>>> u16 proto, const struct sk_buff *skb)
>>>>>> {
>>>>>> struct nf_conntrack_tuple tuple;
>>>>>> + struct nf_conntrack_expect *exp;
>>>>>>
>>>>>> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple))
>>>>>> return NULL;
>>>>>> - return __nf_ct_expect_find(net, zone, &tuple);
>>>>>> +
>>>>>> + exp = __nf_ct_expect_find(net, zone, &tuple);
>>>>>> +
>>>>>
>>>>> Extraneous whitespace^
>>>>>
>>>>
>>>> You mean the empty line?
>>>
>>> Yeah.
>>
>> I can remove this here before applying if that is fine to you, so you
>> don't need to resubmit.
>>
>
> Yes, fine by me! Thank you!
>
> Jarno
>
>>>>>> + if (exp) {
>>>>>> + struct nf_conntrack_tuple_hash *h;
>>>>>> +
>>>>>> + /* Delete existing conntrack entry, if it clashes with the
>>>>>> + * expectation. This can happen since conntrack ALGs do not
>>>>>> + * check for clashes between (new) expectations and existing
>>>>>> + * conntrack entries. nf_conntrack_in() will check the
>>>>>> + * expectations only if a conntrack entry can not be found,
>>>>>> + * which can lead to OVS finding the expectation (here) in the
>>>>>> + * init direction, but which will not be removed by the
>>>>>> + * nf_conntrack_in() call, if a matching conntrack entry is
>>>>>> + * found instead. In this case all init direction packets
>>>>>> + * would be reported as new related packets, while reply
>>>>>> + * direction packets would be reported as un-related
>>>>>> + * established packets. */
>>>>>> +
>>>>>
>>>>> Extraneous whitespace^
>>
>> We're converging to netdev comment style, ie.
>>
>> * ...
>> * established packets.
>> */
>>
>> I know we have a bunch of comments in netfilter ending like the one
>> above, but ideally it would be good to use this comment style.
>>
>>>>>
>>>>>> + h = nf_conntrack_find_get(net, zone, &tuple);
>>>>>> + if (h) {
>>>>>> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
>>>>>> +
>>>>>> + nf_ct_delete(ct, 0, 0);
>>>>>> + nf_conntrack_put(&ct->ct_general);
>>>>>
>>>>> Do we need the extra nf_conntrack_put() here? If
>>>>> nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete()
>>>>> which releases a reference on the CT entry.
>>>>
>>>> There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get().
>>>
>>> Ah, makes sense.
>>>
>>> Acked-by: Joe Stringer <joe@xxxxxxx>
>>>
>>> Did you intend for Pablo to take this? Pablo, is this fine or should
>>> Jarno resubmit against net?
>>
>> I can take this, yes.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
