* [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 13:40 ` Colin King 0 siblings, 0 replies; 14+ messages in thread From: Colin King @ 2021-05-26 13:40 UTC (permalink / raw) To: Christine Caulfield, David Teigland, Alexander Aring, cluster-devel Cc: kernel-janitors, linux-kernel From: Colin Ian King <colin.king@canonical.com> 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@canonical.com> --- 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); return; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 13:40 ` Colin King 0 siblings, 0 replies; 14+ messages in thread From: Colin King @ 2021-05-26 13:40 UTC (permalink / raw) To: cluster-devel.redhat.com From: Colin Ian King <colin.king@canonical.com> 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@canonical.com> --- 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); return; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 13:40 ` [Cluster-devel] " Colin King @ 2021-05-26 14:19 ` Alexander Ahring Oder Aring -1 siblings, 0 replies; 14+ messages in thread From: Alexander Ahring Oder Aring @ 2021-05-26 14:19 UTC (permalink / raw) To: Colin King Cc: Christine Caulfield, David Teigland, cluster-devel, kernel-janitors, linux-kernel Hi, On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > 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@canonical.com> > --- > 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); > return; This seems to be a bigger issue, we cannot revert the buffer allocation with a kfree, we cannot revert it at all. We should avoid any error handling between create_rcom() and send_rcom(). In general between get_buffer/commit_buffer. I don't see a problem with moving the error handling before create_rcom(). That should fix the issue. - Alex ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 14:19 ` Alexander Ahring Oder Aring 0 siblings, 0 replies; 14+ messages in thread From: Alexander Ahring Oder Aring @ 2021-05-26 14:19 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > 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@canonical.com> > --- > 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); > return; This seems to be a bigger issue, we cannot revert the buffer allocation with a kfree, we cannot revert it at all. We should avoid any error handling between create_rcom() and send_rcom(). In general between get_buffer/commit_buffer. I don't see a problem with moving the error handling before create_rcom(). That should fix the issue. - Alex ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 14:19 ` [Cluster-devel] " Alexander Ahring Oder Aring @ 2021-05-26 14:21 ` Colin Ian King -1 siblings, 0 replies; 14+ messages in thread From: Colin Ian King @ 2021-05-26 14:21 UTC (permalink / raw) To: Alexander Ahring Oder Aring Cc: Christine Caulfield, David Teigland, cluster-devel, kernel-janitors, linux-kernel On 26/05/2021 15:19, Alexander Ahring Oder Aring wrote: > Hi, > > On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote: >> >> From: Colin Ian King <colin.king@canonical.com> >> >> 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@canonical.com> >> --- >> 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); >> return; > > This seems to be a bigger issue, we cannot revert the buffer > allocation with a kfree, we cannot revert it at all. We should avoid > any error handling between create_rcom() and send_rcom(). In general > between get_buffer/commit_buffer. > > I don't see a problem with moving the error handling before > create_rcom(). That should fix the issue. Good point, I'll send a V2 in a while > > - Alex > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 14:21 ` Colin Ian King 0 siblings, 0 replies; 14+ messages in thread From: Colin Ian King @ 2021-05-26 14:21 UTC (permalink / raw) To: cluster-devel.redhat.com On 26/05/2021 15:19, Alexander Ahring Oder Aring wrote: > Hi, > > On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote: >> >> From: Colin Ian King <colin.king@canonical.com> >> >> 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@canonical.com> >> --- >> 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); >> return; > > This seems to be a bigger issue, we cannot revert the buffer > allocation with a kfree, we cannot revert it at all. We should avoid > any error handling between create_rcom() and send_rcom(). In general > between get_buffer/commit_buffer. > > I don't see a problem with moving the error handling before > create_rcom(). That should fix the issue. Good point, I'll send a V2 in a while > > - Alex > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 13:40 ` [Cluster-devel] " Colin King @ 2021-05-26 15:01 ` Dan Carpenter -1 siblings, 0 replies; 14+ messages in thread From: Dan Carpenter @ 2021-05-26 15:01 UTC (permalink / raw) To: Colin King Cc: Christine Caulfield, David Teigland, Alexander Aring, cluster-devel, kernel-janitors, linux-kernel On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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@canonical.com> > --- > 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. regards, dan carpenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 15:01 ` Dan Carpenter 0 siblings, 0 replies; 14+ messages in thread From: Dan Carpenter @ 2021-05-26 15:01 UTC (permalink / raw) To: cluster-devel.redhat.com On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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@canonical.com> > --- > 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. regards, dan carpenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 15:01 ` [Cluster-devel] " Dan Carpenter @ 2021-05-26 15:11 ` Colin Ian King -1 siblings, 0 replies; 14+ messages in thread From: Colin Ian King @ 2021-05-26 15:11 UTC (permalink / raw) To: Dan Carpenter Cc: Christine Caulfield, David Teigland, Alexander Aring, cluster-devel, kernel-janitors, linux-kernel 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@canonical.com> >> >> 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@canonical.com> >> --- >> 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. I've send a V2. It was indeed a brown-paper-bag bad fix. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 15:11 ` Colin Ian King 0 siblings, 0 replies; 14+ messages in thread From: Colin Ian King @ 2021-05-26 15:11 UTC (permalink / raw) To: cluster-devel.redhat.com 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@canonical.com> >> >> 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@canonical.com> >> --- >> 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. I've send a V2. It was indeed a brown-paper-bag bad fix. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 15:11 ` [Cluster-devel] " Colin Ian King @ 2021-05-26 18:24 ` Dan Carpenter -1 siblings, 0 replies; 14+ messages in thread From: Dan Carpenter @ 2021-05-26 18:24 UTC (permalink / raw) To: Colin Ian King Cc: Christine Caulfield, David Teigland, Alexander Aring, cluster-devel, kernel-janitors, linux-kernel 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@canonical.com> > >> > >> 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@canonical.com> > >> --- > >> 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". The dlm code seems to be going out of its way to use void pointers and that makes it difficult to parse with Smatch. 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 regards, dan carpenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 18:24 ` Dan Carpenter 0 siblings, 0 replies; 14+ messages in thread From: Dan Carpenter @ 2021-05-26 18:24 UTC (permalink / raw) To: cluster-devel.redhat.com 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@canonical.com> > >> > >> 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@canonical.com> > >> --- > >> 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". The dlm code seems to be going out of its way to use void pointers and that makes it difficult to parse with Smatch. 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 regards, dan carpenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][next] fs: dlm: Fix memory leak of object mh 2021-05-26 18:24 ` [Cluster-devel] " Dan Carpenter @ 2021-05-26 18:50 ` Alexander Ahring Oder Aring -1 siblings, 0 replies; 14+ messages in thread From: Alexander Ahring Oder Aring @ 2021-05-26 18:50 UTC (permalink / raw) To: Dan Carpenter Cc: Colin Ian King, Christine Caulfield, David Teigland, cluster-devel, kernel-janitors, linux-kernel Hi, On Wed, May 26, 2021 at 2:24 PM Dan Carpenter <dan.carpenter@oracle.com> 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@canonical.com> > > >> > > >> 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@canonical.com> > > >> --- > > >> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [PATCH][next] fs: dlm: Fix memory leak of object mh @ 2021-05-26 18:50 ` Alexander Ahring Oder Aring 0 siblings, 0 replies; 14+ messages in thread From: Alexander Ahring Oder Aring @ 2021-05-26 18:50 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, On Wed, May 26, 2021 at 2:24 PM Dan Carpenter <dan.carpenter@oracle.com> 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@canonical.com> > > >> > > >> 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@canonical.com> > > >> --- > > >> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-26 18:50 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-26 13:40 [PATCH][next] fs: dlm: Fix memory leak of object mh Colin King 2021-05-26 13:40 ` [Cluster-devel] " Colin King 2021-05-26 14:19 ` Alexander Ahring Oder Aring 2021-05-26 14:19 ` [Cluster-devel] " Alexander Ahring Oder Aring 2021-05-26 14:21 ` Colin Ian King 2021-05-26 14:21 ` [Cluster-devel] " Colin Ian King 2021-05-26 15:01 ` Dan Carpenter 2021-05-26 15:01 ` [Cluster-devel] " Dan Carpenter 2021-05-26 15:11 ` Colin Ian King 2021-05-26 15:11 ` [Cluster-devel] " Colin Ian King 2021-05-26 18:24 ` Dan Carpenter 2021-05-26 18:24 ` [Cluster-devel] " Dan Carpenter 2021-05-26 18:50 ` Alexander Ahring Oder Aring 2021-05-26 18:50 ` [Cluster-devel] " Alexander Ahring Oder Aring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.