> On Mar 3, 2017, at 1:44 PM, Joe Stringer <[email protected]> wrote:
>
>
>
> On 3/03/2017 10:37, "Jarno Rajahalme" <[email protected] <mailto:[email protected]>>
> wrote:
>
>> On Mar 2, 2017, at 5:26 PM, Joe Stringer <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>> On 28 February 2017 at 17:17, Jarno Rajahalme <[email protected]
>> <mailto:[email protected]>> wrote:
>>> Upstream commit:
>>>
>>> Refactoring conntrack labels initialization makes changes in later
>>> patches easier to review.
>>>
>>> Signed-off-by: Jarno Rajahalme <[email protected] <mailto:[email protected]>>
>>> Acked-by: Pravin B Shelar <[email protected] <mailto:[email protected]>>
>>> Acked-by: Joe Stringer <[email protected] <mailto:[email protected]>>
>>> Signed-off-by: David S. Miller <[email protected]
>>> <mailto:[email protected]>>
>>>
>>> Signed-off-by: Jarno Rajahalme <[email protected] <mailto:[email protected]>>
>>> ---
>>> datapath/conntrack.c | 113
>>> ++++++++++++++++++++++++++++++---------------------
>>> 1 file changed, 66 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
>>> index dacf34c..adc4315 100644
>>> --- a/datapath/conntrack.c
>>> +++ b/datapath/conntrack.c
>>> @@ -243,19 +243,12 @@ 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;
>>> @@ -270,56 +263,71 @@ 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;
>>> -
>>> - /* 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 && ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN)
>>> + return NULL;
>>
>> The above two lines were not introduced in the upstream code. Do you
>> intend to introduce them?
>>
>
> Should have mentioned this in a commit message or in a comment. The inclusion
> of this test is intentional, and the rationale is that it might be possible
> that the kernel is configured with too little space for labels. However, it
> is possible that the way OVS kernel module initializes the number of words in
> labels for older kernels already takes care of this, do you have a take on
> this?
>
> When we compile the out of tree module for a particular kernel, this
> information should be available. I don't think that we try to support
> compiling against one kernel with one definition of the labels length, then
> allow that same module to run on another kernel with a different definition.
> So it should be fine to omit so long as there are still the compile time
> checks.
>
But my understanding is that the compile time checks only apply to newer
kernels, where the available storage for labels is a compile time
configuration, rather than a run-time number of words.
Jarno
>
> Jarno
>
>> For my current working tree for review/build/test, I will drop these lines.
>>
>>> + return cl;
>>> +}
>>> +
>>> +/* Initialize labels for a new, yet 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.
>>> + */
>>> +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;
>>>
>>> - if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN)
>>> + cl = ovs_ct_get_conn_labels(ct);
>>> + if (!cl)
>>> return -ENOSPC;
>>>
>>> - if (nf_ct_is_confirmed(ct)) {
>>> - /* Triggers a change event, which makes sense only for
>>> - * confirmed connections.
>>> - */
>>> - int err = nf_connlabels_replace(ct, labels->ct_labels_32,
>>> - mask->ct_labels_32,
>>> - OVS_CT_LABELS_LEN_32);
>>> - if (err)
>>> - return err;
>>> - } else {
>>> - u32 *dst = (u32 *)cl->bits;
>>> - const u32 *msk = mask->ct_labels_32;
>>> - const u32 *lbl = labels->ct_labels_32;
>>> - int i;
>>> + dst = (u32 *)cl->bits;
>>> + 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]);
>>>
>>> - /* No-one else has access to the non-confirmed entry, copy
>>> - * labels over, keeping any bits we are not explicitly
>>> setting.
>>> - */
>>> - for (i = 0; i < OVS_CT_LABELS_LEN_32; i++)
>>> - dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]);
>>> + /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the
>>> + * IPCT_LABEL bit it set in the event cache.
>>> + */
>>> + nf_conntrack_event_cache(IPCT_LABEL, ct);
>>>
>>> - /* Labels are included in the IPCTNL_MSG_CT_NEW event only
>>> if
>>> - * the IPCT_LABEL bit it set in the event cache.
>>> - */
>>> - nf_conntrack_event_cache(IPCT_LABEL, ct);
>>> - }
>>> + 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,
>>> + mask->ct_labels_32,
>>> + OVS_CT_LABELS_LEN_32);
>>> + if (err)
>>> + return err;
>>> +
>>> + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN);
>>>
>>> - ovs_ct_get_labels(ct, &key->ct.labels);
>>> return 0;
>>> }
>>>
>>> @@ -926,25 +934,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
>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected] <mailto:[email protected]>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
