The title should have been:
openvswitch: Only set mark and labels with a commit flag.
This reflects the fact that modifying the mark and/or labels of an existing
connection is allowed. The commit flag is still required to do that, though.
Jarno
> On Jun 20, 2016, at 5:19 PM, Jarno Rajahalme <ja...@ovn.org> wrote:
>
> Only allow setting 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 on the kernel API.
>
> Finally, set conntrack mark and labels right before committing so that
> the initial conntrack NEW event has the mark and labels.
>
> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> ---
> net/openvswitch/conntrack.c | 72 ++++++++++++++++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3d5feed..f1612f5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -824,6 +824,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,
> @@ -834,24 +845,35 @@ 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;
> - /* This is a no-op if the connection has already been confirmed. */
> +
> + /* As any changes to an unconfirmed connection may be lost due
> + * to an upcall, we require the presence of the 'commit' flag
> + * for setting mask and/or labels. Perform the changes before
> + * confirming the connection so that the initial mark and label
> + * values are present in the initial CT NEW netlink event.
> + */
> + 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;
> + }
> +
> + /* Will take care of sending queued events even if the connection is
> + * already confirmed.
> + */
> if (nf_conntrack_confirm(skb) != NF_ACCEPT)
> return -EINVAL;
>
> 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;
> -}
> -
> /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
> * value if 'skb' is freed.
> */
> @@ -876,19 +898,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> err = ovs_ct_commit(net, key, info, skb);
> else
> err = ovs_ct_lookup(net, key, info, skb);
> - if (err)
> - goto err;
>
> - 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);
> -err:
> skb_push(skb, nh_ofs);
> if (err)
> kfree_skb(skb);
> @@ -1145,6 +1155,20 @@ static int parse_ct(const struct nlattr *attr, struct
> ovs_conntrack_info *info,
> }
> }
>
> +#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);
> return -EINVAL;
> --
> 2.1.4
>
