netdev - Re: [patch net-next 02/12] ipmr: Add reference count to MFC entries

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: <54cb367e-3769-ce9f-27dc-e08ee710bd98@cumulusnetworks.com>
Date:   Thu, 21 Sep 2017 14:56:59 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, yotamg@...lanox.com, idosch@...lanox.com,
        mlxsw@...lanox.com
Subject: Re: [patch net-next 02/12] ipmr: Add reference count to MFC entries

On 21/09/17 09:43, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@...lanox.com>
> 
> Next commits will introduce MFC notifications through the atomic
> fib_notification chain, thus allowing modules to be aware of MFC entries.
> 
> Due to the fact that modules may need to hold a reference to an MFC entry,
> add reference count to MFC entries to prevent them from being freed while
> these modules use them.
> 
> The reference counting is done only on resolved MFC entries currently.
> 
> Signed-off-by: Yotam Gigi <yotamg@...lanox.com>
> Reviewed-by: Ido Schimmel <idosch@...lanox.com>
> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> ---
>  include/linux/mroute.h | 20 ++++++++++++++++++++
>  net/ipv4/ipmr.c        |  8 +++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/mroute.h b/include/linux/mroute.h
> index d7f6333..2f88e3d 100644
> --- a/include/linux/mroute.h
> +++ b/include/linux/mroute.h
> @@ -138,6 +138,7 @@ struct mfc_cache {
>  			unsigned long wrong_if;
>  			unsigned long lastuse;
>  			unsigned char ttls[MAXVIFS];
> +			refcount_t refcount;

There's a struct comment above that has a short description for each entry,
I know it seems redundant, but could you please add one for this ?

>  		} res;
>  	} mfc_un;
>  	struct list_head list;
> @@ -148,4 +149,23 @@ struct rtmsg;
>  int ipmr_get_route(struct net *net, struct sk_buff *skb,
>  		   __be32 saddr, __be32 daddr,
>  		   struct rtmsg *rtm, u32 portid);
> +
> +#ifdef CONFIG_IP_MROUTE
> +void ipmr_cache_free(struct mfc_cache *mfc_cache);
> +#else
> +static inline void ipmr_cache_free(struct mfc_cache *mfc_cache)
> +{
> +}
> +#endif
> +
> +static inline void ipmr_cache_put(struct mfc_cache *c)
> +{
> +	if (refcount_dec_and_test(&c->mfc_un.res.refcount))
> +		ipmr_cache_free(c);
> +}
> +static inline void ipmr_cache_hold(struct mfc_cache *c)
> +{
> +	refcount_inc(&c->mfc_un.res.refcount);
> +}
> +
>  #endif
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index c9b3e6e..86dc5f9 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -652,10 +652,11 @@ static void ipmr_cache_free_rcu(struct rcu_head *head)
>  	kmem_cache_free(mrt_cachep, c);
>  }
>  
> -static inline void ipmr_cache_free(struct mfc_cache *c)
> +void ipmr_cache_free(struct mfc_cache *c)
>  {
>  	call_rcu(&c->rcu, ipmr_cache_free_rcu);
>  }
> +EXPORT_SYMBOL(ipmr_cache_free);
>  
>  /* Destroy an unresolved cache entry, killing queued skbs
>   * and reporting error to netlink readers.
> @@ -949,6 +950,7 @@ static struct mfc_cache *ipmr_cache_alloc(void)
>  	if (c) {
>  		c->mfc_un.res.last_assert = jiffies - MFC_ASSERT_THRESH - 1;
>  		c->mfc_un.res.minvif = MAXVIFS;
> +		refcount_set(&c->mfc_un.res.refcount, 1);
>  	}
>  	return c;
>  }
> @@ -1162,7 +1164,7 @@ static int ipmr_mfc_delete(struct mr_table *mrt, struct mfcctl *mfc, int parent)
>  	rhltable_remove(&mrt->mfc_hash, &c->mnode, ipmr_rht_params);
>  	list_del_rcu(&c->list);
>  	mroute_netlink_event(mrt, c, RTM_DELROUTE);
> -	ipmr_cache_free(c);
> +	ipmr_cache_put(c);
>  
>  	return 0;
>  }
> @@ -1264,7 +1266,7 @@ static void mroute_clean_tables(struct mr_table *mrt, bool all)
>  		rhltable_remove(&mrt->mfc_hash, &c->mnode, ipmr_rht_params);
>  		list_del_rcu(&c->list);
>  		mroute_netlink_event(mrt, c, RTM_DELROUTE);
> -		ipmr_cache_free(c);
> +		ipmr_cache_put(c);
>  	}
>  
>  	if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
>