| Message ID | 1467398276-115931-2-git-send-email-pshelar@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
With a small nit below: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Jul 1, 2016, at 11:37 AM, Pravin B Shelar <pshelar@ovn.org> wrote: > > Upstream commit: > commit 7d904c7bcd51f72579c0c3134a50896c5a3efb9f > Author: Jarno Rajahalme <jarno@ovn.org> > Date: Tue Jun 21 14:59:38 2016 -0700 > > openvswitch: Only set mark and labels with a commit flag. > > Only set conntrack mark or labels when the commit flag is specified. > This makes sure we can not set them before the connection has been > persisted, as in that case the mark and labels would be lost in an > event of an userspace upcall. > > OVS userspace already requires the commit flag to accept setting > ct_mark and/or ct_labels. Validate for this in the kernel API. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Signed-off-by: Pravin B Shelar <pshelar@ovn.org> > --- > datapath/conntrack.c | 79 +++++++++++++++++++++++++++++++--------------------- > 1 file changed, 47 insertions(+), 32 deletions(-) > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index 1ef6828..22324c1 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -464,6 +464,17 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key, > return 0; > } > > +static bool labels_nonzero(const struct ovs_key_ct_labels *labels) > +{ > + size_t i; > + > + for (i = 0; i < sizeof(*labels); i++) > + if (labels->ct_labels[i]) > + return true; > + > + return false; > +} > + > /* Lookup connection and confirm if unconfirmed. */ > static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, > const struct ovs_conntrack_info *info, > @@ -484,18 +495,31 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, > err = __ovs_ct_lookup(net, key, info, skb); > if (err) > return err; > - return 0; > -} > > -static bool labels_nonzero(const struct ovs_key_ct_labels *labels) > -{ > - size_t i; > + /* Apply changes before confirming the connection so that the initial > + * conntrack NEW netlink event carries the values given in the CT > + * action. > + */ > > - for (i = 0; i < sizeof(*labels); i++) > - if (labels->ct_labels[i]) > - return true; > + if (info->mark.mask) { > + err = ovs_ct_set_mark(skb, 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 (err) > + return err; > + } > + /* This will take care of sending queued events even if the connection > + * is already confirmed. > + */ > + if (nf_conntrack_confirm(skb) != NF_ACCEPT) > + return -EINVAL; > > - return false; > + return 0; > } > > /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero > @@ -525,29 +549,6 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb, > if (err) > goto err; > This branch seems unnecessary now, as the label is on the next line. > - /* 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, > - info->mark.mask); > - if (err) > - goto err; > - } > - if (labels_nonzero(&info->labels.mask)) { > - err = ovs_ct_set_labels(skb, key, &info->labels.value, > - &info->labels.mask); > - if (err) > - goto err; > - } > - /* This will take care of sending queued events even if the connection > - * is already confirmed. > - */ > - if (info->commit && nf_conntrack_confirm(skb) != NF_ACCEPT) > - err = -EINVAL; > - > err: > skb_push(skb, nh_ofs); > if (err) > @@ -661,6 +662,20 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info, > return -EINVAL; > } > } > +#ifdef CONFIG_NF_CONNTRACK_MARK > + if (!info->commit && info->mark.mask) { > + OVS_NLERR(log, > + "Setting conntrack mark requires 'commit' flag."); > + return -EINVAL; > + } > +#endif > +#ifdef CONFIG_NF_CONNTRACK_LABELS > + if (!info->commit && labels_nonzero(&info->labels.mask)) { > + OVS_NLERR(log, > + "Setting conntrack labels requires 'commit' flag."); > + return -EINVAL; > + } > +#endif > > if (rem > 0) { > OVS_NLERR(log, "Conntrack attr has %d unknown bytes", rem); > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
diff --git a/datapath/conntrack.c b/datapath/conntrack.c index 1ef6828..22324c1 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -464,6 +464,17 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key, return 0; } +static bool labels_nonzero(const struct ovs_key_ct_labels *labels) +{ + size_t i; + + for (i = 0; i < sizeof(*labels); i++) + if (labels->ct_labels[i]) + return true; + + return false; +} + /* Lookup connection and confirm if unconfirmed. */ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, const struct ovs_conntrack_info *info, @@ -484,18 +495,31 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, err = __ovs_ct_lookup(net, key, info, skb); if (err) return err; - return 0; -} -static bool labels_nonzero(const struct ovs_key_ct_labels *labels) -{ - size_t i; + /* Apply changes before confirming the connection so that the initial + * conntrack NEW netlink event carries the values given in the CT + * action. + */ - for (i = 0; i < sizeof(*labels); i++) - if (labels->ct_labels[i]) - return true; + if (info->mark.mask) { + err = ovs_ct_set_mark(skb, 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 (err) + return err; + } + /* This will take care of sending queued events even if the connection + * is already confirmed. + */ + if (nf_conntrack_confirm(skb) != NF_ACCEPT) + return -EINVAL; - return false; + return 0; } /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero @@ -525,29 +549,6 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb, if (err) goto err; - /* 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, - info->mark.mask); - if (err) - goto err; - } - if (labels_nonzero(&info->labels.mask)) { - err = ovs_ct_set_labels(skb, key, &info->labels.value, - &info->labels.mask); - if (err) - goto err; - } - /* This will take care of sending queued events even if the connection - * is already confirmed. - */ - if (info->commit && nf_conntrack_confirm(skb) != NF_ACCEPT) - err = -EINVAL; - err: skb_push(skb, nh_ofs); if (err) @@ -661,6 +662,20 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info, return -EINVAL; } } +#ifdef CONFIG_NF_CONNTRACK_MARK + if (!info->commit && info->mark.mask) { + OVS_NLERR(log, + "Setting conntrack mark requires 'commit' flag."); + return -EINVAL; + } +#endif +#ifdef CONFIG_NF_CONNTRACK_LABELS + if (!info->commit && labels_nonzero(&info->labels.mask)) { + OVS_NLERR(log, + "Setting conntrack labels requires 'commit' flag."); + return -EINVAL; + } +#endif if (rem > 0) { OVS_NLERR(log, "Conntrack attr has %d unknown bytes", rem);
Upstream commit: commit 7d904c7bcd51f72579c0c3134a50896c5a3efb9f Author: Jarno Rajahalme <jarno@ovn.org> Date: Tue Jun 21 14:59:38 2016 -0700 openvswitch: Only set mark and labels with a commit flag. Only set conntrack mark or labels when the commit flag is specified. This makes sure we can not set them before the connection has been persisted, as in that case the mark and labels would be lost in an event of an userspace upcall. OVS userspace already requires the commit flag to accept setting ct_mark and/or ct_labels. Validate for this in the kernel API. Signed-off-by: Jarno Rajahalme <jarno@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> --- datapath/conntrack.c | 79 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 32 deletions(-)