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]
Message-Id: <7773486D-AC7A-4ABE-B0E5-3229CBE56C08@ovn.org>
Date:   Tue, 7 Feb 2017 21:30:54 -0800
From:   Jarno Rajahalme <jarno@....org>
To:     Joe Stringer <joe@....org>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/7] openvswitch: Do not trigger events for unconfirmed connection.

Thanks for the review! Comments below,

  Jarno

> On Feb 6, 2017, at 1:46 PM, Joe Stringer <joe@....org> wrote:
> 
> On 2 February 2017 at 17:10, Jarno Rajahalme <jarno@....org> wrote:
>> Avoid triggering change events for setting conntrack mark or labels
>> before the conntrack entry has been confirmed.  Refactoring on this
>> patch also makes chenges in later patches easier to review.
>> 
>> Fixes: 182e3042e15d ("openvswitch: Allow matching on conntrack mark")
>> Fixes: c2ac66735870 ("openvswitch: Allow matching on conntrack label")
>> Signed-off-by: Jarno Rajahalme <jarno@....org>
> 
> Functional and cosmetic changes should be in separate patches.
> 

OK, will split.

>> ---
>> net/openvswitch/conntrack.c | 87 ++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 63 insertions(+), 24 deletions(-)
>> 
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index 6730f09..a163c44 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -229,23 +229,17 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
>>        return 0;
>> }
>> 
>> -static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key,
>> +static int ovs_ct_set_mark(struct nf_conn *ct, struct sw_flow_key *key,
>>                           u32 ct_mark, u32 mask)
>> {
>> #if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK)
>> -       enum ip_conntrack_info ctinfo;
>> -       struct nf_conn *ct;
>>        u32 new_mark;
>> 
>> -       /* The connection could be invalid, in which case set_mark is no-op. */
>> -       ct = nf_ct_get(skb, &ctinfo);
>> -       if (!ct)
>> -               return 0;
>> -
>>        new_mark = ct_mark | (ct->mark & ~(mask));
>>        if (ct->mark != new_mark) {
>>                ct->mark = new_mark;
>> -               nf_conntrack_event_cache(IPCT_MARK, ct);
>> +               if (nf_ct_is_confirmed(ct))
>> +                       nf_conntrack_event_cache(IPCT_MARK, ct);
>>                key->ct.mark = new_mark;
>>        }
>> 
>> @@ -255,26 +249,59 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key,
>> #endif
>> }
>> 
>> -static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key,
>> -                            const struct ovs_key_ct_labels *labels,
>> -                            const struct ovs_key_ct_labels *mask)
>> +static struct nf_conn_labels *ovs_ct_get_conn_labels(struct nf_conn *ct)
>> {
>> -       enum ip_conntrack_info ctinfo;
>>        struct nf_conn_labels *cl;
>> -       struct nf_conn *ct;
>> -       int err;
>> -
>> -       /* The connection could be invalid, in which case set_label is no-op.*/
>> -       ct = nf_ct_get(skb, &ctinfo);
>> -       if (!ct)
>> -               return 0;
>> 
>>        cl = nf_ct_labels_find(ct);
>>        if (!cl) {
>>                nf_ct_labels_ext_add(ct);
>>                cl = nf_ct_labels_find(ct);
>>        }
>> +
>>        if (!cl || sizeof(cl->bits) < OVS_CT_LABELS_LEN)
>> +               return NULL;
>> +
>> +       return cl;
>> +}
>> +
>> +/* Initialize labels for a new, to be committed conntrack entry.  Note that
>> + * since the new connection is not yet confirmed, and thus no-one else has
>> + * access to it's labels, we simply write them over.  Also, we refrain from
>> + * triggering events, as receiving change events before the create event would
>> + * be confusing.
>> + */
>> +static int ovs_ct_init_labels(struct nf_conn *ct, struct sw_flow_key *key,
>> +                             const struct ovs_key_ct_labels *labels,
>> +                             const struct ovs_key_ct_labels *mask)
>> +{
>> +       struct nf_conn_labels *cl;
>> +       u32 *dst;
>> +       int i;
>> +
>> +       cl = ovs_ct_get_conn_labels(ct);
>> +       if (!cl)
>> +               return -ENOSPC;
>> +
>> +       dst = (u32 *)cl->bits;
> 
> Is it worth extending the union to include unsigned long, to avoid
> casting it to u32 here?
> 

This cast is on the struct nf_conn_labels, I would not unionize it at this point. This type of cast is typical in conntrack code.

>> +       for (i = 0; i < OVS_CT_LABELS_LEN_32; i++)
>> +               dst[i] = (dst[i] & ~mask->ct_labels_32[i]) |
>> +                       (labels->ct_labels_32[i] & mask->ct_labels_32[i]);
>> +
>> +       memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN);
>> +
>> +       return 0;
>> +}
>> +
>> +static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key,
>> +                            const struct ovs_key_ct_labels *labels,
>> +                            const struct ovs_key_ct_labels *mask)
>> +{
>> +       struct nf_conn_labels *cl;
>> +       int err;
>> +
>> +       cl = ovs_ct_get_conn_labels(ct);
>> +       if (!cl)
>>                return -ENOSPC;
>> 
>>        err = nf_connlabels_replace(ct, labels->ct_labels_32,
>> @@ -283,7 +310,8 @@ static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key,
>>        if (err)
>>                return err;
>> 
>> -       ovs_ct_get_labels(ct, &key->ct.labels);
>> +       memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN);
>> +
> 
> I noticed this change and started looking at ovs_ct_get_labels(). It
> tries to handle length differences between nf_conn_labels matches
> ovs_key_ct_labels, but perhaps it's easier to drop that code and use a
> build-time assert that they're the same instead. Since 23014011ba42
> ("netfilter: conntrack: support a fixed size of 128 distinct labels"),
> they haven't been variable length anyway so this can simplify some
> code. This can be separate from this patch though.
> 
> I think that such a change would be orthogonal from the above change,
> the above can stay as is (not much point sharing the function call if
> it just retrieves a pointer we already have and does a memcpy).
> 

I kept this change with the refactoring patch, not with the functional change patch.

>>        return 0;
>> }
>> 
>> @@ -865,25 +893,36 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>>                         const struct ovs_conntrack_info *info,
>>                         struct sk_buff *skb)
>> {
>> +       enum ip_conntrack_info ctinfo;
>> +       struct nf_conn *ct;
>>        int err;
>> 
>>        err = __ovs_ct_lookup(net, key, info, skb);
>>        if (err)
>>                return err;
>> 
>> +       /* The connection could be invalid, in which case this is a no-op.*/
>> +       ct = nf_ct_get(skb, &ctinfo);
>> +       if (!ct)
>> +               return 0;
>> +
>>        /* Apply changes before confirming the connection so that the initial
>>         * conntrack NEW netlink event carries the values given in the CT
>>         * action.
>>         */
>>        if (info->mark.mask) {
>> -               err = ovs_ct_set_mark(skb, key, info->mark.value,
>> +               err = ovs_ct_set_mark(ct, key, info->mark.value,
>>                                      info->mark.mask);
>>                if (err)
>>                        return err;
>>        }
>>        if (labels_nonzero(&info->labels.mask)) {
>> -               err = ovs_ct_set_labels(skb, key, &info->labels.value,
>> -                                       &info->labels.mask);
>> +               if (!nf_ct_is_confirmed(ct))
>> +                       err = ovs_ct_init_labels(ct, key, &info->labels.value,
>> +                                                &info->labels.mask);
>> +               else
>> +                       err = ovs_ct_set_labels(ct, key, &info->labels.value,
>> +                                               &info->labels.mask);
>>                if (err)
>>                        return err;
>>        }
>> --
>> 2.1.4

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ