Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Alexander Ahring Oder Aring]

Hi,

On Wed, May 26, 2021 at 2:24 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Wed, May 26, 2021 at 04:11:06PM +0100, Colin Ian King wrote:
> > On 26/05/2021 16:01, Dan Carpenter wrote:
> > > On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
> > >> From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> > >>
> > >> There is an error return path that is not kfree'ing mh after
> > >> it has been successfully allocates.  Fix this by free'ing it.
> > >>
> > >> Addresses-Coverity: ("Resource leak")
> > >> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> > >> Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> > >> ---
> > >>  fs/dlm/rcom.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> > >> index 085f21966c72..19298edc1573 100644
> > >> --- a/fs/dlm/rcom.c
> > >> +++ b/fs/dlm/rcom.c
> > >> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
> > >>    if (rc_in->rc_id == 0xFFFFFFFF) {
> > >>            log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
> > >>            dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> > >> +          kfree(mh);
> > >
> > > Am I looking at the same code as you?  (I often am not able to review
> > > your patches because you're doing development on stuff that hasn't hit
> > > linux-next).  Anyway, to me this doesn't seem like the correct fix at
> > > all.  There are some other things to free and the "mh" pointer is on
> > > a bunch of lists so it leads to use after frees.
>       ^^^^^^^^^^^^^^
> This is sort of impossible, of course, because the struct only has one
> list_head so it can only be in one list and not a "bunch of lists".
>

It is a bunch of lists because mh_handle holds pointers with ref
counters to other structures which are part of lists. :) There is a
list_del() only if hits zero.

> The dlm code seems to be going out of its way to use void pointers and
> that makes it difficult to parse with Smatch.
>

That has been changed on dlm/next. There exists a struct mh_handle *
and a dlm_msg * to get rid of void * handles.

> But in other subsystems, we could make it a rule that list_heads are
> "poison" "init" or "added".  If you freed a memory with an "added"
> list_head then print a warning.  Or if you added a list_head but it was
> already in the added state then print a warning.  Another idea is that
> if you freed a struct mh before the mh->page allocation was freed then
> print a warning about the leak.  This one is probably more prone to
> false positives but there might be workarounds for those.  #IdeasToImplement
>

Currently if a buffer is allocated it is not possible to free it
again. The allocated buffer of the page will be transmitted
(kernel_sendpage()) out in a contiguous way. If somebody wants to
release memory the page buffer needs to be reordered and it can only
be done before commit().

- Alex