From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathias Krause Subject: Re: [net] xfrm_user: use xfrm_state_put to free xfrm_state_alloc return value Date: Wed, 21 Nov 2018 21:09:23 +0100 Message-ID: <20181121200923.GA12460@jig.fritz.box> References: <1542783468-67482-1-git-send-email-bianpan2016@163.com> <20181121080045.4vtozqc6eyeyis2n@gondor.apana.org.au> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Pan Bian , "David S. Miller" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Pan Bian , minipli@googlemail.com To: Herbert Xu , Steffen Klassert Return-path: Content-Disposition: inline In-Reply-To: <20181121080045.4vtozqc6eyeyis2n@gondor.apana.org.au> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Wed, Nov 21, 2018 at 04:00:45PM +0800, Herbert Xu wrote: > On Wed, Nov 21, 2018 at 02:57:48PM +0800, Pan Bian wrote: > > From: Pan Bian > > > > The memory chunk allocated by xfrm_state_alloc() should be released with > > xfrm_state_put(), not kfree. > > > > Signed-off-by: Pan Bian > > This bug was introduced by > > commit 565f0fa902b64020d5d147ff1708567e9e0b6e49 > Author: Mathias Krause > Date: Thu May 3 10:55:07 2018 +0200 > Oh, snap. You're totally right. I missed the kfree() in xfrm_user.c. Sorry for that! > While using xfrm_state_put may work it's certainly not the designed > to do this. We should instead export a function that calls > kmem_cache_free on xfrm_state directly and use that here. Maybe something like the below patch? Steffen? -- >8 -- Subject: [PATCH] xfrm_user: fix freeing of xfrm states on acquire Commit 565f0fa902b6 ("xfrm: use a dedicated slab cache for struct xfrm_state") moved xfrm state objects to use their own slab cache. However, it missed to adapt xfrm_user to use this new cache when freeing xfrm states. Fix this by introducing and make use of a new helper for freeing xfrm_state objects. Fixes: 565f0fa902b6 ("xfrm: use a dedicated slab cache for struct xfrm_state") Reported-by: Pan Bian Cc: # v4.18+ Signed-off-by: Mathias Krause --- include/net/xfrm.h | 1 + net/xfrm/xfrm_state.c | 8 +++++++- net/xfrm/xfrm_user.c | 4 ++-- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 0eb390c205af..da588def3c61 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1552,6 +1552,7 @@ int xfrm_state_walk(struct net *net, struct xfrm_state_walk *walk, int (*func)(struct xfrm_state *, int, void*), void *); void xfrm_state_walk_done(struct xfrm_state_walk *walk, struct net *net); struct xfrm_state *xfrm_state_alloc(struct net *net); +void xfrm_state_free(struct xfrm_state *x); struct xfrm_state *xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, const struct flowi *fl, diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index dc4a9f1fb941..0a0b01b688d7 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -426,6 +426,12 @@ static void xfrm_put_mode(struct xfrm_mode *mode) module_put(mode->owner); } +void xfrm_state_free(struct xfrm_state *x) +{ + kmem_cache_free(xfrm_state_cache, x); +} +EXPORT_SYMBOL(xfrm_state_free); + static void xfrm_state_gc_destroy(struct xfrm_state *x) { tasklet_hrtimer_cancel(&x->mtimer); @@ -452,7 +458,7 @@ static void xfrm_state_gc_destroy(struct xfrm_state *x) } xfrm_dev_state_free(x); security_xfrm_state_free(x); - kmem_cache_free(xfrm_state_cache, x); + xfrm_state_free(x); } static void xfrm_state_gc_task(struct work_struct *work) diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index c9a84e22f5d5..277c1c46fe94 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2288,13 +2288,13 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh, } - kfree(x); + xfrm_state_free(x); kfree(xp); return 0; free_state: - kfree(x); + xfrm_state_free(x); nomem: return err; }