Intentional memory leak in ipmi_msghandler?
PATCH 1/1 Intentional memory leak in ipmi_msghandler?
Hello,
I've got a few boxes that are leaking memory in handle_new_recv_msgs() in ipmi_msghandler. AFAICS this is intentional, there's even an explicit counter that tracks the number of times smi_msg is leaked.
I'm guessing there was a reason for doing this, but there wasn't any discussion about it on LKML when the patch was accepted. Can you clarify why something like the below patch won't work? I tried it on one of my leaky boxes and nothing obviously horrible happened.
Thanks,
Calvin
----8<----
From: Calvin Owens <[email protected]>
Subject: [PATCH] ipmi_msghandler: Don't leak memory on errors
--- drivers/char/ipmi/ipmi_msghandler.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/char/ipmi/ipmi_msghandler.c +1 -4
@@ -3834,10 +3834,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)break ; }else {list_del (&smi_msg->link); -if (rv ==0 ) -/* Message handled */ -ipmi_free_smi_msg (smi_msg); -/* If rv < 0, fatal error, del but don't free. */ +ipmi_free_smi_msg (smi_msg); } }if (!run_to_completion)
Signature · 1 line
2.4.6
Quoted 5 lines · Calvin Owens
Hello,
I've got a few boxes that are leaking memory in
handle_new_recv_msgs()inipmi_msghandler. AFAICS this is intentional, there's even an explicit counter that tracks the number of timessmi_msgis leaked.
Are you 100% sure about this? There's no intentional leak, a negative return from this function means the message was used for another purpose and thus shouldn't be freed. There's only one situation where this happens and you should never hit it in normal operation.
Quoted 4 lines
I'm guessing there was a reason for doing this, but there wasn't any discussion about it on LKML when the patch was accepted. Can you clarify why something like the below patch won't work? I tried it on one of my leaky boxes and nothing obviously horrible happened.
Well, that's because nothing probably happened, and it probably had no effect on the leak. A better comment on this code is probably in order. But that patch is incorrect.
I doubt the leak is here. If you are having a leak, is it possible to characterize it better? Are you handling commands from IPMB? Are you handling LAN commands here?
-corey
Quoted 29 lines
Thanks,
Calvin----8<----
From: Calvin Owens <[email protected]>
Subject: [PATCH]ipmi_msghandler: Don't leak memory on errorsSigned-off-by Calvin Owens <[email protected]>--- drivers/char/ipmi/ipmi_msghandler.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)drivers/char/ipmi/ipmi_msghandler.c +1
-4@@ -3834,10 +3834,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)break ; }else {list_del (&smi_msg->link); -if (rv ==0 ) -/* Message handled */ -ipmi_free_smi_msg (smi_msg); -/* If rv < 0, fatal error, del but don't free. */ +ipmi_free_smi_msg (smi_msg); } }if (!run_to_completion)
Quoted 8 lines · Corey Minyard
Quoted 5 lines · Calvin Owens
Hello,
I've got a few boxes that are leaking memory in
handle_new_recv_msgs()inipmi_msghandler. AFAICS this is intentional, there's even an explicit counter that tracks the number of timessmi_msgis leaked.Are you 100% sure about this?
I'm absolutely certain this is where I'm leaking: I threw in a printk() and saw exact correlation between the number of times I saw that and the number of kmalloc-1024 objects leaked. But...
Quoted 4 lines
There's no intentional leak, a negative return from this function means the message was used for another purpose and thus shouldn't be freed. There's only one situation where this happens and you should never hit it in normal operation.
This is actually extremely helpful: we have some horrible non-upstream code behind this that I thought I had ruled out being at fault here, but this sounds like it might be. In any case, I won't waste your time any more until I can reproduce it on upstream (which is unfortunately impossible on this particular HW I see the leak on, otherwise I would have done that in the first place...).
Thanks very much for the prompt response, I really appreciate it.
Calvin
Quoted 45 lines
Quoted 4 lines
I'm guessing there was a reason for doing this, but there wasn't any discussion about it on LKML when the patch was accepted. Can you clarify why something like the below patch won't work? I tried it on one of my leaky boxes and nothing obviously horrible happened.
Well, that's because nothing probably happened, and it probably had no effect on the leak. A better comment on this code is probably in order. But that patch is incorrect.
I doubt the leak is here. If you are having a leak, is it possible to characterize it better? Are you handling commands from IPMB? Are you handling LAN commands here?
-corey
Quoted 29 lines
Thanks,
Calvin----8<----
From: Calvin Owens <[email protected]>
Subject: [PATCH]ipmi_msghandler: Don't leak memory on errorsSigned-off-by Calvin Owens <[email protected]>--- drivers/char/ipmi/ipmi_msghandler.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)drivers/char/ipmi/ipmi_msghandler.c +1
-4@@ -3834,10 +3834,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)break ; }else {list_del (&smi_msg->link); -if (rv ==0 ) -/* Message handled */ -ipmi_free_smi_msg (smi_msg); -/* If rv < 0, fatal error, del but don't free. */ +ipmi_free_smi_msg (smi_msg); } }if (!run_to_completion)