On 3 March 2017 at 16:04, Jarno Rajahalme <[email protected]> wrote: > > On Mar 3, 2017, at 1:44 PM, Joe Stringer <[email protected]> wrote: > > > > On 3/03/2017 10:37, "Jarno Rajahalme" <[email protected]> wrote: > > > On Mar 2, 2017, at 5:26 PM, Joe Stringer <[email protected]> wrote: > > On 28 February 2017 at 17:17, Jarno Rajahalme <[email protected]> wrote: > > Upstream commit: > > Refactoring conntrack labels initialization makes changes in later > patches easier to review. > > Signed-off-by: Jarno Rajahalme <[email protected]> > Acked-by: Pravin B Shelar <[email protected]> > Acked-by: Joe Stringer <[email protected]> > Signed-off-by: David S. Miller <[email protected]> > > Signed-off-by: Jarno Rajahalme <[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.
OK, there's a couple of pieces here: The compile time checks check that the connlabels support up to the size that OVS needs. Kernels up up until 3.15 do not define the NF_CT_LABELS_MAX_SIZE in the headers, but allows labels up until 128 bits. Kernels 3.15 up until 4.9 define the above, allowing labels up until 128 bits. Kernels 4.9 and above also allow up to 128 bits. Then runtime: Kernels up until 4.9 will dynamically change the labels length to the largest of all users within the namespace. Kernels 4.9 and above will always set the labels length to 128 bits - so when the labels extension is added to the conntrack entry, it's always big enough. At module load time, openvswitch attempts to set connlabel 127 (via nf_connlabels_get()) to ensure that connlabels extensions are added to conntarck entries with enough space to satisfy the OVS CT_LABELS API. There's a small possibility that someone configures an older kernel with a lower number of labels, runs connections through, then loads the OVS module, then OVS looks up an existing CT entry and finds that entry with a lower labels size. If we want to cover this, then perhaps we can shift the two lines that we're discussing into the implementation of nf_ct_labels_find() in the compat code, activated by a check that the kernel hasn't yet included upstream commit 23014011ba4209a086931ff402eac1c41abbe456. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
