[BUG] net: rds: rds_send_probe memory leak

[BUG] net: rds: rds_send_probe memory leak

[Fatih Yildirim]

Hi Santosh,

I've been working on a memory leak bug reported by syzbot.
https://syzkaller.appspot.com/bug?id=39b72114839a6dbd66c1d2104522698a813f9ae2

It seems that memory allocated in rds_send_probe function is not freed.

Let me share my observations.
rds_message is allocated at the beginning of rds_send_probe function.
Then it is added to cp_send_queue list of rds_conn_path and refcount
is increased by one.
Next, in rds_send_xmit function it is moved from cp_send_queue list to
cp_retrans list, and again refcount is increased by one.
Finally in rds_loop_xmit function refcount is increased by one.
So, total refcount is 4.
However, rds_message_put is called three times, in rds_send_probe,
rds_send_remove_from_sock and rds_send_xmit functions. It seems that
one more rds_message_put is needed.
Would you please check and share your comments on this issue?

Thanks,
Fatih

Re: [BUG] net: rds: rds_send_probe memory leak

[Greg KH]

On Sun, Mar 14, 2021 at 11:23:10AM +0300, Fatih Yildirim wrote:
> Hi Santosh,
> 
> I've been working on a memory leak bug reported by syzbot.
> https://syzkaller.appspot.com/bug?id=39b72114839a6dbd66c1d2104522698a813f9ae2
> 
> It seems that memory allocated in rds_send_probe function is not freed.
> 
> Let me share my observations.
> rds_message is allocated at the beginning of rds_send_probe function.
> Then it is added to cp_send_queue list of rds_conn_path and refcount
> is increased by one.
> Next, in rds_send_xmit function it is moved from cp_send_queue list to
> cp_retrans list, and again refcount is increased by one.
> Finally in rds_loop_xmit function refcount is increased by one.
> So, total refcount is 4.
> However, rds_message_put is called three times, in rds_send_probe,
> rds_send_remove_from_sock and rds_send_xmit functions. It seems that
> one more rds_message_put is needed.
> Would you please check and share your comments on this issue?

Do you have a proposed patch that syzbot can test to verify if this is
correct or not?

thanks,

gre gk-h

Re: [BUG] net: rds: rds_send_probe memory leak

[Fatih Yildirim]

On Sun, 2021-03-14 at 09:36 +0100, Greg KH wrote:
> On Sun, Mar 14, 2021 at 11:23:10AM +0300, Fatih Yildirim wrote:
> > Hi Santosh,
> > 
> > I've been working on a memory leak bug reported by syzbot.
> > https://syzkaller.appspot.com/bug?id=39b72114839a6dbd66c1d2104522698a813f9ae2
> > 
> > It seems that memory allocated in rds_send_probe function is not
> > freed.
> > 
> > Let me share my observations.
> > rds_message is allocated at the beginning of rds_send_probe
> > function.
> > Then it is added to cp_send_queue list of rds_conn_path and
> > refcount
> > is increased by one.
> > Next, in rds_send_xmit function it is moved from cp_send_queue list
> > to
> > cp_retrans list, and again refcount is increased by one.
> > Finally in rds_loop_xmit function refcount is increased by one.
> > So, total refcount is 4.
> > However, rds_message_put is called three times, in rds_send_probe,
> > rds_send_remove_from_sock and rds_send_xmit functions. It seems
> > that
> > one more rds_message_put is needed.
> > Would you please check and share your comments on this issue?
> 
> Do you have a proposed patch that syzbot can test to verify if this
> is
> correct or not?
> 
> thanks,
> 
> gre gk-h

Hi Greg,

Actually, using the .config and the C reproducer, syzbot reports the
memory leak in rds_send_probe function. Also by enabling
CONFIG_RDS_DEBUG=y, the debug messages indicates the similar as I
mentioned above. To give an example, below is the RDS_DEBUG messages.
Allocated address 000000008a7476e5 has initial ref_count 1. Then there
are three rds_message_addref calls for the same address making the
refcount 4, but only three rds_message_put calls which leave the
address still allocated.

[   60.570681] rds_message_addref(): addref rm 000000008a7476e5 ref 1
[   60.570707] rds_message_put(): put rm 000000008a7476e5 ref 2
[   60.570845] rds_message_addref(): addref rm 000000008a7476e5 ref 1
[   60.570870] rds_message_addref(): addref rm 000000008a7476e5 ref 2
[   60.570960] rds_message_put(): put rm 000000008a7476e5 ref 3
[   60.570995] rds_message_put(): put rm 000000008a7476e5 ref 2

Thanks,
Fatih

Re: [BUG] net: rds: rds_send_probe memory leak

[Greg KH]

On Sun, Mar 14, 2021 at 03:19:05PM +0300, Fatih Yildirim wrote:
> On Sun, 2021-03-14 at 09:36 +0100, Greg KH wrote:
> > On Sun, Mar 14, 2021 at 11:23:10AM +0300, Fatih Yildirim wrote:
> > > Hi Santosh,
> > > 
> > > I've been working on a memory leak bug reported by syzbot.
> > > https://syzkaller.appspot.com/bug?id=39b72114839a6dbd66c1d2104522698a813f9ae2
> > > 
> > > It seems that memory allocated in rds_send_probe function is not
> > > freed.
> > > 
> > > Let me share my observations.
> > > rds_message is allocated at the beginning of rds_send_probe
> > > function.
> > > Then it is added to cp_send_queue list of rds_conn_path and
> > > refcount
> > > is increased by one.
> > > Next, in rds_send_xmit function it is moved from cp_send_queue list
> > > to
> > > cp_retrans list, and again refcount is increased by one.
> > > Finally in rds_loop_xmit function refcount is increased by one.
> > > So, total refcount is 4.
> > > However, rds_message_put is called three times, in rds_send_probe,
> > > rds_send_remove_from_sock and rds_send_xmit functions. It seems
> > > that
> > > one more rds_message_put is needed.
> > > Would you please check and share your comments on this issue?
> > 
> > Do you have a proposed patch that syzbot can test to verify if this
> > is
> > correct or not?
> > 
> > thanks,
> > 
> > gre gk-h
> 
> Hi Greg,
> 
> Actually, using the .config and the C reproducer, syzbot reports the
> memory leak in rds_send_probe function. Also by enabling
> CONFIG_RDS_DEBUG=y, the debug messages indicates the similar as I
> mentioned above. To give an example, below is the RDS_DEBUG messages.
> Allocated address 000000008a7476e5 has initial ref_count 1. Then there
> are three rds_message_addref calls for the same address making the
> refcount 4, but only three rds_message_put calls which leave the
> address still allocated.
> 
> [   60.570681] rds_message_addref(): addref rm 000000008a7476e5 ref 1
> [   60.570707] rds_message_put(): put rm 000000008a7476e5 ref 2
> [   60.570845] rds_message_addref(): addref rm 000000008a7476e5 ref 1
> [   60.570870] rds_message_addref(): addref rm 000000008a7476e5 ref 2
> [   60.570960] rds_message_put(): put rm 000000008a7476e5 ref 3
> [   60.570995] rds_message_put(): put rm 000000008a7476e5 ref 2
> 

Ok, so the next step is to try your proposed change to see if it works
or not.  What prevents you from doign that?

No need to ask people if your analysis of an issue is true or not, no
maintainer or developer usually has the time to deal with that.  We much
rather would like to see patches of things you have tested to resolve
issues.

thanks,

greg k-h

Re: [BUG] net: rds: rds_send_probe memory leak

[Fatih Yildirim]

On Sun, 2021-03-14 at 13:44 +0100, Greg KH wrote:
> On Sun, Mar 14, 2021 at 03:19:05PM +0300, Fatih Yildirim wrote:
> > On Sun, 2021-03-14 at 09:36 +0100, Greg KH wrote:
> > > On Sun, Mar 14, 2021 at 11:23:10AM +0300, Fatih Yildirim wrote:
> > > > Hi Santosh,
> > > > 
> > > > I've been working on a memory leak bug reported by syzbot.
> > > > https://syzkaller.appspot.com/bug?id=39b72114839a6dbd66c1d2104522698a813f9ae2
> > > > 
> > > > It seems that memory allocated in rds_send_probe function is
> > > > not
> > > > freed.
> > > > 
> > > > Let me share my observations.
> > > > rds_message is allocated at the beginning of rds_send_probe
> > > > function.
> > > > Then it is added to cp_send_queue list of rds_conn_path and
> > > > refcount
> > > > is increased by one.
> > > > Next, in rds_send_xmit function it is moved from cp_send_queue
> > > > list
> > > > to
> > > > cp_retrans list, and again refcount is increased by one.
> > > > Finally in rds_loop_xmit function refcount is increased by one.
> > > > So, total refcount is 4.
> > > > However, rds_message_put is called three times, in
> > > > rds_send_probe,
> > > > rds_send_remove_from_sock and rds_send_xmit functions. It seems
> > > > that
> > > > one more rds_message_put is needed.
> > > > Would you please check and share your comments on this issue?
> > > 
> > > Do you have a proposed patch that syzbot can test to verify if
> > > this
> > > is
> > > correct or not?
> > > 
> > > thanks,
> > > 
> > > gre gk-h
> > 
> > Hi Greg,
> > 
> > Actually, using the .config and the C reproducer, syzbot reports
> > the
> > memory leak in rds_send_probe function. Also by enabling
> > CONFIG_RDS_DEBUG=y, the debug messages indicates the similar as I
> > mentioned above. To give an example, below is the RDS_DEBUG
> > messages.
> > Allocated address 000000008a7476e5 has initial ref_count 1. Then
> > there
> > are three rds_message_addref calls for the same address making the
> > refcount 4, but only three rds_message_put calls which leave the
> > address still allocated.
> > 
> > [   60.570681] rds_message_addref(): addref rm 000000008a7476e5 ref
> > 1
> > [   60.570707] rds_message_put(): put rm 000000008a7476e5 ref 2
> > [   60.570845] rds_message_addref(): addref rm 000000008a7476e5 ref
> > 1
> > [   60.570870] rds_message_addref(): addref rm 000000008a7476e5 ref
> > 2
> > [   60.570960] rds_message_put(): put rm 000000008a7476e5 ref 3
> > [   60.570995] rds_message_put(): put rm 000000008a7476e5 ref 2
> > 
> 
> Ok, so the next step is to try your proposed change to see if it
> works
> or not.  What prevents you from doign that?
> 
> No need to ask people if your analysis of an issue is true or not, no
> maintainer or developer usually has the time to deal with that.  We
> much
> rather would like to see patches of things you have tested to resolve
> issues.
> 
> thanks,
> 
> greg k-h

Hi Greg,

I also would like to come with a patch to resolve the issue as well.
But couldn't figure out so far. I just would like to have a review or a
suggestion from an expert in order to move forward.
Anyway, I'm still working on it and hope to find a solution.
Will appreciate any comment, suggestion on the issue.

Thanks,
Fatih