On Thu, Feb 25, 2016 at 12:19:01PM +0300, Dan Carpenter wrote:
> On Thu, Feb 25, 2016 at 11:11:45AM +0200, Amir Vadai" wrote:
> > Hi,
> >
> > I have a kernel patch that I would like to post upstream. I get a memory leak
> > warning on it, it is a false positive. See below for a simplified example:
> >
> > When runing:
> > $ ~/git/smatch/smatch_scripts/kchecker smatch_test.c
> >
> > I get this:
> >
> > smatch_test.c:11 test() warn: possible memory leak of 'a'
> > CC smatch_test.o
> >
> >
> > On the below trival code:
> >
> > #include <linux/slab.h>
> >
> > int test(void)
> > {
> > void *a = kmalloc(1000, GFP_KERNEL);
> > bool f = false;
> >
> > if (!f)
> > kfree(a);
>
> This is like what Oleg was asking about last week. We know that this
> condition is true and actually Smatch knows this condition is true as
Thanks for the quick reply.
I'm sorry - I couldn't find an archive for the smatch mailing list and
I'm not registered on it. I've only found an old archive at
sourceforge.
> well. But it is a tricky question of how to handle these things where
> we know the condition is true. It's tricky from a philosophical point
> of view as well as an implementation point of view.
>
> Why do we have this known condition true? Wouldn't it be cleaner to
> just remove it?
>
> >
> > return 0;
> > }
>
> Also you have to be really careful when writing test cases. How sure
> are you that "bool" and "false" are defined? Smatch won't complain
> about it, but can lead to wierd results.
>
> Can you send the patch itself?
See the patch below (it is one patch from a full patchset - but the
situtation itself is clear).
In the function mlx5e_configure_flower(), smatch warn that 'flow' might
leak.
'flow' is allocated only if the same key exists in the hashtable. In
that case 'old' will be non zero, and kfree won't be called. This is
what makes smatch angry...
I guess I can replace the
if (!old)
kfree()
with:
if (old)
flow = NULL;
kfree(flow)
But it looks ugly...
Here is the patch:
>From 286229b5a25530f3b0592f88fd033533aa6a3710 Mon Sep 17 00:00:00 2001
From: Amir Vadai <amir@xxxxxxxx>
Date: Tue, 12 Jan 2016 12:51:28 +0200
Subject: [PATCH net-next V8 INT 7/9] net/mlx5e: Support offload cls_flower
with drop action
Parse tc_cls_flower_offload into device specific commands and program
the hardware to classify and act accordingly.
For example, to drop ICMP (ip_proto 1) packets from specific smac, dmac,
src_ip, src_ip, arriving to interface ens9:
# tc qdisc add dev ens9 ingress
# tc filter add dev ens9 protocol ip parent ffff: \
flower eth_type ip ip_proto 1 \
dst_mac 7c:fe:90:69:81:62 src_mac 7c:fe:90:69:81:56 \
dst_ip 11.11.11.11 src_ip 11.11.11.12 indev ens9 \
action drop
Signed-off-by: Amir Vadai <amir@xxxxxxxx>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 10 +
.../net/ethernet/mellanox/mlx5/core/en_offload.c | 302 +++++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/en_offload.h | 17 ++
3 files changed, 329 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index adcb09f..bf228ad 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -30,6 +30,7 @@
* SOFTWARE.
*/
+#include <net/tc_act/tc_gact.h>
#include <net/pkt_cls.h>
#include <linux/mlx5/fs.h>
#include <net/vxlan.h>
@@ -1889,6 +1890,15 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
goto mqprio;
switch (tc->type) {
+ case TC_SETUP_CLSFLOWER:
+ switch (tc->cls_flower->command) {
+ case TC_CLSFLOWER_REPLACE:
+ return mlx5e_configure_flower(priv, proto, tc->cls_flower);
+ case TC_CLSFLOWER_DESTROY:
+ return mlx5e_delete_flower(priv, tc->cls_flower);
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_offload.c b/drivers/net/ethernet/mellanox/mlx5/core/en_offload.c
index 99882b9..1fe3e43 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_offload.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_offload.c
@@ -30,6 +30,9 @@
* SOFTWARE.
*/
+#include <net/flow_dissector.h>
+#include <net/pkt_cls.h>
+#include <net/tc_act/tc_gact.h>
#include <linux/mlx5/fs.h>
#include <linux/mlx5/device.h>
#include <linux/rhashtable.h>
@@ -107,6 +110,305 @@ static void mlx5e_del_offload(struct mlx5e_priv *priv,
}
}
+static int parse_cls_flower(u32 *match_c, u32 *match_v,
+ struct tc_cls_flower_offload *f)
+{
+ void *headers_c = MLX5_ADDR_OF(fte_match_param, match_c, outer_headers);
+ void *headers_v = MLX5_ADDR_OF(fte_match_param, match_v, outer_headers);
+ u16 addr_type = 0;
+ u8 ip_proto = 0;
+
+ if (f->dissector->used_keys &
+ ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
+ BIT(FLOW_DISSECTOR_KEY_BASIC) |
+ BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
+ BIT(FLOW_DISSECTOR_KEY_VLANID) |
+ BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
+ BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
+ BIT(FLOW_DISSECTOR_KEY_PORTS))) {
+ pr_warn("Unsupported key used: 0x%x\n",
+ f->dissector->used_keys);
+ return -ENOTSUPP;
+ }
+
+ if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_CONTROL)) {
+ struct flow_dissector_key_control *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_BASIC,
+ f->key);
+ addr_type = key->addr_type;
+ }
+
+ if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
+ struct flow_dissector_key_basic *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_BASIC,
+ f->key);
+ struct flow_dissector_key_basic *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_BASIC,
+ f->mask);
+ ip_proto = key->ip_proto;
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, ethertype,
+ ntohs(mask->n_proto));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, ethertype,
+ ntohs(key->n_proto));
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, ip_protocol,
+ mask->ip_proto);
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, ip_protocol,
+ key->ip_proto);
+ }
+
+ if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+ struct flow_dissector_key_eth_addrs *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_ETH_ADDRS,
+ f->key);
+ struct flow_dissector_key_eth_addrs *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_ETH_ADDRS,
+ f->mask);
+
+ ether_addr_copy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ dmac_47_16),
+ mask->dst);
+ ether_addr_copy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ dmac_47_16),
+ key->dst);
+
+ ether_addr_copy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ smac_47_16),
+ mask->src);
+ ether_addr_copy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ smac_47_16),
+ key->src);
+ }
+
+ if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_VLANID)) {
+ struct flow_dissector_key_tags *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_VLANID,
+ f->key);
+ struct flow_dissector_key_tags *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_VLANID,
+ f->mask);
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, vlan_tag, 1);
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, vlan_tag, 1);
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_vid,
+ ntohs(mask->vlan_id));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_vid,
+ ntohs(key->vlan_id));
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_cfi,
+ ntohs(mask->flow_label));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_cfi,
+ ntohs(key->flow_label));
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c, first_prio,
+ ntohs(mask->flow_label) >> 1);
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v, first_prio,
+ ntohs(key->flow_label) >> 1);
+ }
+
+ if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
+ struct flow_dissector_key_ipv4_addrs *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+ f->key);
+ struct flow_dissector_key_ipv4_addrs *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+ f->mask);
+
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ src_ipv4_src_ipv6.ipv4_layout.ipv4),
+ &mask->src, sizeof(mask->src));
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ src_ipv4_src_ipv6.ipv4_layout.ipv4),
+ &key->src, sizeof(key->src));
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ dst_ipv4_dst_ipv6.ipv4_layout.ipv4),
+ &mask->dst, sizeof(mask->dst));
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ dst_ipv4_dst_ipv6.ipv4_layout.ipv4),
+ &key->dst, sizeof(key->dst));
+ }
+
+ if (addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS) {
+ struct flow_dissector_key_ipv6_addrs *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+ f->key);
+ struct flow_dissector_key_ipv6_addrs *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+ f->mask);
+
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ src_ipv4_src_ipv6.ipv6_layout.ipv6),
+ &mask->src, sizeof(mask->src));
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ src_ipv4_src_ipv6.ipv6_layout.ipv6),
+ &key->src, sizeof(key->src));
+
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_c,
+ dst_ipv4_dst_ipv6.ipv6_layout.ipv6),
+ &mask->dst, sizeof(mask->dst));
+ memcpy(MLX5_ADDR_OF(fte_match_set_lyr_2_4, headers_v,
+ dst_ipv4_dst_ipv6.ipv6_layout.ipv6),
+ &key->dst, sizeof(key->dst));
+ }
+
+ if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_PORTS)) {
+ struct flow_dissector_key_ports *key =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_PORTS,
+ f->key);
+ struct flow_dissector_key_ports *mask =
+ skb_flow_dissector_target(f->dissector,
+ FLOW_DISSECTOR_KEY_PORTS,
+ f->mask);
+ switch (ip_proto) {
+ case IPPROTO_TCP:
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c,
+ tcp_sport, ntohs(mask->src));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v,
+ tcp_sport, ntohs(key->src));
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c,
+ tcp_dport, ntohs(mask->dst));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v,
+ tcp_dport, ntohs(key->dst));
+ break;
+
+ case IPPROTO_UDP:
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c,
+ udp_sport, ntohs(mask->src));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v,
+ udp_sport, ntohs(key->src));
+
+ MLX5_SET(fte_match_set_lyr_2_4, headers_c,
+ udp_dport, ntohs(mask->dst));
+ MLX5_SET(fte_match_set_lyr_2_4, headers_v,
+ udp_dport, ntohs(key->dst));
+ break;
+ default:
+ pr_err("Only UDP and TCP transport are supported\n");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int parse_tc_actions(u32 *action, u32 *flow_tag,
+ struct tcf_exts *exts)
+{
+ const struct tc_action *a;
+
+ if (list_empty(&exts->actions))
+ return -EINVAL;
+
+ list_for_each_entry(a, &exts->actions, list) {
+ if (is_tcf_gact_shot(a)) {
+ *action |= MLX5_FLOW_CONTEXT_ACTION_DROP;
+ continue;
+ }
+
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int mlx5e_configure_flower(struct mlx5e_priv *priv, __be16 protocol,
+ struct tc_cls_flower_offload *f)
+{
+ struct mlx5e_offloads_flow_table *offloads = &priv->fts.offloads;
+ u32 *match_c;
+ u32 *match_v;
+ int err = 0;
+ u32 flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
+ u32 action = 0;
+ struct mlx5e_offload_flow *flow;
+ struct mlx5_flow_rule *old = NULL;
+
+ flow = rhashtable_lookup_fast(&offloads->ht, &f->handle,
+ offloads->ht_params);
+ if (flow)
+ old = flow->rule;
+ else
+ flow = kzalloc(sizeof(*flow), GFP_KERNEL);
+
+ match_c = kzalloc(MLX5_ST_SZ_BYTES(fte_match_param), GFP_KERNEL);
+ match_v = kzalloc(MLX5_ST_SZ_BYTES(fte_match_param), GFP_KERNEL);
+ if (!match_c || !match_v || !flow) {
+ err = -ENOMEM;
+ goto err_free;
+ }
+
+ flow->handle = f->handle;
+
+ err = parse_cls_flower(match_c, match_v, f);
+ if (err < 0)
+ goto err_free;
+
+ err = parse_tc_actions(&action, &flow_tag, f->exts);
+ if (err < 0)
+ goto err_free;
+
+ err = rhashtable_insert_fast(&offloads->ht, &flow->node,
+ offloads->ht_params);
+ if (err) {
+ mlx5e_del_offload(priv, flow->rule);
+ goto err_free;
+ }
+
+ err = mlx5e_add_offload(priv, &flow->rule, match_c, match_v, action,
+ flow_tag);
+ if (err)
+ goto err_free;
+
+ if (old)
+ mlx5e_del_offload(priv, old);
+
+ goto out;
+
+err_free:
+ if (!old)
+ kfree(flow);
+out:
+ kfree(match_c);
+ kfree(match_v);
+ return err;
+}
+
+int mlx5e_delete_flower(struct mlx5e_priv *priv,
+ struct tc_cls_flower_offload *f)
+{
+ struct mlx5e_offload_flow *flow;
+ struct mlx5e_offloads_flow_table *offloads = &priv->fts.offloads;
+
+ flow = rhashtable_lookup_fast(&offloads->ht, &f->handle,
+ offloads->ht_params);
+ if (!flow) {
+ pr_err("Can't find requested flow");
+ return -EINVAL;
+ }
+
+ rhashtable_remove_fast(&offloads->ht, &flow->node, offloads->ht_params);
+
+ mlx5e_del_offload(priv, flow->rule);
+
+ kfree(flow);
+
+ return 0;
+}
+
static const struct rhashtable_params mlx5e_offload_flow_ht_params = {
.head_offset = offsetof(struct mlx5e_offload_flow, node),
.key_offset = offsetof(struct mlx5e_offload_flow, handle),
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_offload.h b/drivers/net/ethernet/mellanox/mlx5/core/en_offload.h
index 7cb9f93..32f1fb6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_offload.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_offload.h
@@ -36,12 +36,29 @@
#ifdef CONFIG_NET_CLS_ACT
void mlx5e_offload_init(struct net_device *dev);
+int mlx5e_configure_flower(struct mlx5e_priv *priv, __be16 protocol,
+ struct tc_cls_flower_offload *f);
+int mlx5e_delete_flower(struct mlx5e_priv *priv,
+ struct tc_cls_flower_offload *f);
+
#else
static inline void mlx5e_offload_init(struct net_device *dev)
{
}
+static inline int mlx5e_configure_flower(struct mlx5e_priv *priv, __be16 protocol,
+ struct tc_cls_flower_offload *f)
+{
+ return -ENOTSUPP;
+}
+
+static inline int mlx5e_delete_flower(struct mlx5e_priv *priv,
+ struct tc_cls_flower_offload *f)
+{
+ return -ENOTSUPP;
+}
+
#endif
static inline int mlx5e_num_offloaded_filters(struct mlx5e_priv *priv)
--
2.7.0
--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
