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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150330062632.GB2045@nanopsycho.orion>
Date:	Mon, 30 Mar 2015 08:26:32 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, tgraf@...g.ch,
	jesse@...ira.com
Subject: Re: [patch net-next] tc: introduce OpenFlow classifier

Mon, Mar 30, 2015 at 02:39:31AM CEST, jhs@...atatu.com wrote:
>Jiri,
>
>Comments below
>
>On 03/26/15 08:53, Jiri Pirko wrote:
>>This patch introduces OpenFlow-based filter. So far, the very essential
>>packet fields are supported (according to OpenFlow v1.4 spec).
>>
>>This patch is only the first step. There is a lot of potential performance
>>improvements possible to implement. Also a lot of features are missing
>>now. They will be addressed in follow-up patches.
>>
>>To the name of this classifier, I believe that "cls_openflow" is pretty
>>accurate. It is actually a OpenFlow classifier.
>>
>
>
>I agree with your statement above.
>Small comments below:
>
>
>>Signed-off-by: Jiri Pirko <jiri@...nulli.us>
>>---
>
>>+struct of_flow_key {
>>+	int	indev_ifindex;
>>+	struct {
>>+		u8	src[ETH_ALEN];
>>+		u8	dst[ETH_ALEN];
>>+		__be16	type;
>>+	} eth;
>
>
>Could you have got rid of "type" above given we always have
>a "proto" field in tc that is checked at the core?

Sure it is possible. But I try to stick with the names used in OpenFlof
documentation.

>
>
>>+	struct {
>>+		u8	proto;
>>+	} ip;
>>+	union {
>>+		struct {
>>+			__be32 src;
>>+			__be32 dst;
>>+		} ipv4;
>>+		struct {
>>+			struct in6_addr src;
>>+			struct in6_addr dst;
>>+		} ipv6;
>>+	};
>>+	union {
>>+		struct {
>>+			__be16 src;
>>+			__be16 dst;
>>+		} tp;
>>+	};
>
>Was there something else that was intended to go into that tp union?

Following fields are missing:

OXM_OF_SCTP_SRC
OXM_OF_SCTP_DST
OXM_OF_ICMPV4_TYPE
OXM_OF_ICMPV4_CODE


>
>>+static int of_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
>>+		   struct sk_buff *skb, struct tcmsg *t)
>>+{
>>+	struct cls_of_filter *f = (struct cls_of_filter *) fh;
>>+	struct nlattr *nest;
>>+	struct of_flow_key *key, *mask;
>>+
>>+	if (!f)
>>+		return skb->len;
>>+
>>+	t->tcm_handle = f->handle;
>>+
>>+	nest = nla_nest_start(skb, TCA_OPTIONS);
>>+	if (!nest)
>>+		goto nla_put_failure;
>>+
>>+	if (f->res.classid &&
>>+	    nla_put_u32(skb, TCA_BASIC_CLASSID, f->res.classid))
>>+		goto nla_put_failure;
>
>
>I am sure you didnt meant the above;->

Ha. Will fix.

>
>Other than that looks good. I dont mind saying
>Acked-by: Jamal Hadi Salim <jhs@...atatu.com>

Thanks.

>
>General comments:
>so what happens if someone adds a new field? It sounds to me like
>given it is tied to datapath match it will never be backward compatible
>(i.e think old tc, new kernel vs new tc, old kernel)



Well if kernel does not understand the new field, it will simply ignore
it.


>
>cheers,
>jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ