From patchwork Sat Sep 30 08:59:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Shmulik Ladkani X-Patchwork-Id: 820186 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=nsof.io header.i=@nsof.io header.b="y0N68NwR"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3y42SB5v0qz9t2h for ; Sat, 30 Sep 2017 18:59:54 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751289AbdI3I7i (ORCPT ); Sat, 30 Sep 2017 04:59:38 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:50462 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750933AbdI3I7g (ORCPT ); Sat, 30 Sep 2017 04:59:36 -0400 Received: by mail-wm0-f43.google.com with SMTP id u138so2940720wmu.5 for ; Sat, 30 Sep 2017 01:59:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nsof.io; s=google; h=from:to:cc:subject:date:message-id; bh=dAK2pqZKrvKeQgo4ye3pMl3t84hWzIqUpVhk3iXcEEk=; b=y0N68NwR3p+sYOOXfDexXI/NUWT/+JesJLZ9NlCeGLwxluRUgK7ubxBplaRg8fW2T3 eAX5d0aEpv8Npu6HaqTkz4bEKw41R1xWO27m+j4Q4uU8qVuDVtulmA2eTCerKTvJ2dpM y1e/R0+1mcRoeRyfYlSDa1wBknKaTupjQPlR0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=dAK2pqZKrvKeQgo4ye3pMl3t84hWzIqUpVhk3iXcEEk=; b=QvGMtymRMfWMlS3AgxGfxLJGK+s7MrydjoRSxPO9ApogibtVaHQKW/+qlEp54PLGO2 w/6xLGzD5aOXHsHmHMQLT4JaMuIltkrxaae5sQc4VncuYwfwopY0iHn79limot91ValR 9+2c8h2QSplE2CLKwaAI4SJfjnDYNlLNllyZfdNNd84wNVTX49OdD6joszGNgQOtWdnp 15Mm1TVWuoVE52fks/SihWu7FcTWCyMuh5PZqPruj0sbeQje/fcB1xF2o+6hFgDIGaaN MUIQRCKBMQ2r9514pVH16jgvwxERSPi9dCn6D30+wouID9Vm0+8vQ3q5ilodPDRb/OA/ UvfQ== X-Gm-Message-State: AHPjjUjJvFwwoUTZq/lboRO9HJJCWHmEVqRt/+v5ZEZWI4sefzXJrTaQ 3p0Tm1Dd2c1b/HWs5WmP/REB3w== X-Google-Smtp-Source: AOwi7QDUgSWOuX4ZvsjSXqhIl0E1Q2pLPaDPI9F6gCwnmtcxorkY3skltaegyqpfE/AXXvRAGge3Yw== X-Received: by 10.28.57.197 with SMTP id g188mr5222328wma.92.1506761974739; Sat, 30 Sep 2017 01:59:34 -0700 (PDT) Received: from pixies.home ([188.120.148.118]) by smtp.gmail.com with ESMTPSA id m25sm2081875wmi.7.2017.09.30.01.59.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 30 Sep 2017 01:59:34 -0700 (PDT) From: Shmulik Ladkani To: "David S . Miller" , netdev@vger.kernel.org Cc: Mateusz Bajorski , David Ahern , Thomas Graf , Shmulik Ladkani Subject: [PATCH net] net: fib_rules: Fix fib_rules_ops->compare implementations to support exact match Date: Sat, 30 Sep 2017 11:59:09 +0300 Message-Id: <20170930085909.1103-1-shmulik@nsof.io> X-Mailer: git-send-email 2.14.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Shmulik Ladkani Commit 153380ec4b9b ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule") added a check to 'fib_nl_newrule' that tests whether the suggested rule already exists (i.e. has same properties). The check uses fib_rules_ops->compare() method to compare the protocol specific properties. However, the 'compare()' implementations have wildcard semantics: They compared the src_len of the given 'frh' against existing rules ONLY if frh->src_len is specified (i.e. non-zero). Same applies to dst_len and tos fields. The wildcard behavior exists, since formerly (prior 153380ec4b9b), the only use of 'compare' was in fib_nl_delrule, and it was expected that one can delete a rule using a partial match. However, the wildcard behavior is incorrect for fib_nl_newrule: It means that if one specifies 0.0.0.0/0 as the FRA_SRC (or equivalently doesn't specify a FRA_SRC), frh->src_len will NOT be compared against other rules, resulting in compare returning true. This leads to inconsistencies, depending on order of operations, e.g.: A working sequence: # ip ru add from 0.0.0.0/0 iif eth2 pref 22 table 22 # ip ru add from 10.0.0.0/8 iif eth2 pref 22 table 22 # ip ru | grep 22 # both added successfully 22: from all iif eth2 lookup 22 22: from 10.0.0.0/8 iif eth2 lookup 22 A non-working sequence: # ip ru add from 10.0.0.0/8 iif eth2 pref 33 table 33 # ip ru add from 0.0.0.0/0 iif eth2 pref 33 table 33 RTNETLINK answers: File exists # wildcard compare returned true Fix, by adding an 'exact' mode for comparison: In exact mode, frh->src_len is compared to existing rules' src_len regardless whether frh->src_len is non-zero. (same applies for 'dst_len' and 'tos'). Fixes: 153380ec4b9b ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule") Reported-by: Eyal Birger Signed-off-by: Shmulik Ladkani --- An alternative I considered was changing all 'compare' implementations to have "exact" semantics. This has the adavantage of less user confusion, as both NEWRULE and DELRULE have an "exact match" behavior. Alas, I assume there are existing users which might rely on the wildcard behavior of DELRULE. Therefore, this patch results in NEWRULE+NLM_F_EXCL having "exact" semantics, but keeps the "wildcard" semantics for DELRULE. --- include/net/fib_rules.h | 2 +- net/core/fib_rules.c | 4 ++-- net/decnet/dn_rules.c | 6 +++--- net/ipv4/fib_rules.c | 8 ++++---- net/ipv4/ipmr.c | 2 +- net/ipv6/fib6_rules.c | 8 ++++---- net/ipv6/ip6mr.c | 2 +- 7 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/net/fib_rules.h b/include/net/fib_rules.h index 3d7f1cefc6f5..23c406282af8 100644 --- a/include/net/fib_rules.h +++ b/include/net/fib_rules.h @@ -74,7 +74,7 @@ struct fib_rules_ops { int (*delete)(struct fib_rule *); int (*compare)(struct fib_rule *, struct fib_rule_hdr *, - struct nlattr **); + struct nlattr **, bool); int (*fill)(struct fib_rule *, struct sk_buff *, struct fib_rule_hdr *); size_t (*nlmsg_payload)(struct fib_rule *); diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index 9a6d97c1d810..7e5cdee5263a 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -422,7 +422,7 @@ static int rule_exists(struct fib_rules_ops *ops, struct fib_rule_hdr *frh, !uid_eq(r->uid_range.end, rule->uid_range.end)) continue; - if (!ops->compare(r, frh, tb)) + if (!ops->compare(r, frh, tb, true)) continue; return 1; } @@ -702,7 +702,7 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh, !uid_eq(rule->uid_range.end, range.end))) continue; - if (!ops->compare(rule, frh, tb)) + if (!ops->compare(rule, frh, tb, false)) continue; if (rule->flags & FIB_RULE_PERMANENT) { diff --git a/net/decnet/dn_rules.c b/net/decnet/dn_rules.c index 295bbd6a56f2..f8117248564c 100644 --- a/net/decnet/dn_rules.c +++ b/net/decnet/dn_rules.c @@ -158,14 +158,14 @@ static int dn_fib_rule_configure(struct fib_rule *rule, struct sk_buff *skb, } static int dn_fib_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh, - struct nlattr **tb) + struct nlattr **tb, bool exact) { struct dn_fib_rule *r = (struct dn_fib_rule *)rule; - if (frh->src_len && (r->src_len != frh->src_len)) + if ((exact || frh->src_len) && r->src_len != frh->src_len) return 0; - if (frh->dst_len && (r->dst_len != frh->dst_len)) + if ((exact || frh->dst_len) && r->dst_len != frh->dst_len) return 0; if (frh->src_len && (r->src != nla_get_le16(tb[FRA_SRC]))) diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c index 35d646a62ad4..13369b7e247f 100644 --- a/net/ipv4/fib_rules.c +++ b/net/ipv4/fib_rules.c @@ -277,17 +277,17 @@ static int fib4_rule_delete(struct fib_rule *rule) } static int fib4_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh, - struct nlattr **tb) + struct nlattr **tb, bool exact) { struct fib4_rule *rule4 = (struct fib4_rule *) rule; - if (frh->src_len && (rule4->src_len != frh->src_len)) + if ((exact || frh->src_len) && rule4->src_len != frh->src_len) return 0; - if (frh->dst_len && (rule4->dst_len != frh->dst_len)) + if ((exact || frh->dst_len) && rule4->dst_len != frh->dst_len) return 0; - if (frh->tos && (rule4->tos != frh->tos)) + if ((exact || frh->tos) && rule4->tos != frh->tos) return 0; #ifdef CONFIG_IP_ROUTE_CLASSID diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c index 292a8e80bdfa..13d6702d5b8f 100644 --- a/net/ipv4/ipmr.c +++ b/net/ipv4/ipmr.c @@ -192,7 +192,7 @@ static int ipmr_rule_configure(struct fib_rule *rule, struct sk_buff *skb, } static int ipmr_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh, - struct nlattr **tb) + struct nlattr **tb, bool exact) { return 1; } diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c index b240f24a6e52..7ffbff3777ce 100644 --- a/net/ipv6/fib6_rules.c +++ b/net/ipv6/fib6_rules.c @@ -265,17 +265,17 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb, } static int fib6_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh, - struct nlattr **tb) + struct nlattr **tb, bool exact) { struct fib6_rule *rule6 = (struct fib6_rule *) rule; - if (frh->src_len && (rule6->src.plen != frh->src_len)) + if ((exact || frh->src_len) && rule6->src.plen != frh->src_len) return 0; - if (frh->dst_len && (rule6->dst.plen != frh->dst_len)) + if ((exact || frh->dst_len) && rule6->dst.plen != frh->dst_len) return 0; - if (frh->tos && (rule6->tclass != frh->tos)) + if ((exact || frh->tos) && rule6->tclass != frh->tos) return 0; if (frh->src_len && diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index f5500f5444e9..95a0f15c4810 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -196,7 +196,7 @@ static int ip6mr_rule_configure(struct fib_rule *rule, struct sk_buff *skb, } static int ip6mr_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh, - struct nlattr **tb) + struct nlattr **tb, bool exact) { return 1; }