netdev - Re: [ovs-dev] [PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <03B14ADF-5E25-4E63-923A-32DF73D9FFFC@ovn.org>
Date:	Tue, 21 Jun 2016 15:02:34 -0700
From:	Jarno Rajahalme <jarno@....org>
To:	Joe Stringer <joe@....org>
Cc:	netdev <netdev@...r.kernel.org>, ovs dev <dev@...nvswitch.org>
Subject: Re: [ovs-dev] [PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

Thanks for the review!

> On Jun 21, 2016, at 1:57 PM, Joe Stringer <joe@....org> wrote:
> 
> On 20 June 2016 at 17:19, Jarno Rajahalme <jarno@....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 <jarno@....org>
> 
> The structure of this commit message suggests there are multiple
> changes trying to be addressed in one patch. I suggest splitting them
> out.
> 

Done for v2 I just sent for net.

> In terms of applying the mark and labels before committing the
> connection, that's actually the behaviour I would expect if you were
> to execute ct(mark=foo,commit). The NEW event should include these
> pieces, and should have all along.

Right, the v2 patch 1/2 does this.

> 
>> @@ -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
> 
> I'm of mixed minds about this, but I lean towards agreeing with it. On
> one hand, it's applying more restrictions on an otherwise fairly loose
> interface and if anyone is relying on this behaviour then it would be
> surprising to have this restriction introduced. On the other hand, it
> doesn't make a lot of sense to set a label/mark but not to commit the
> connection. As you say, the behaviour isn't exactly consistent in that
> case today anyway: If there was a flow with
> actions=ct(mark=foo),recirc() followed by a userspace upcall, then the
> mark would be reflected in the flow key but not saved to any persisted
> connection. A subsequent ct(commit) after upcall wouldn't persist it,
> either. However if there were two flows already in the datapath to do
> this, then it /would/ be persisted. Restricting the mark/labels
> modification to only if you have the "commit" flag would address that
> consistency issue. The OVS userspace enforcing this constraint also
> hints that this was an unintentional omission from kernel validation.

I separated this out to the v2 patch 2/2.

  Jarno