With a small nit below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jul 1, 2016, at 11:37 AM, Pravin B Shelar <pshe...@ovn.org> wrote:
>
> Upstream commit:
> commit 7d904c7bcd51f72579c0c3134a50896c5a3efb9f
> Author: Jarno Rajahalme <ja...@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 <ja...@ovn.org>
> Signed-off-by: David S. Miller <da...@davemloft.net>
>
> Signed-off-by: Pravin B Shelar <pshe...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
