We should check the ring allocations don't fail.
If we get a fail we need to clean up properly. The allocator assumes the
deallocator will be used on failure, but it isn't. Make sure the
right deallocator is always called and add a missing check against
fbr allocation failure.
[v2]: Correct check logic
Signed-off-by: Alan Cox <[email protected]>
---
drivers/staging/et131x/et131x.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 6413500..cc600df 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
/* Alloc memory for the lookup table */
rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
+ if (rx_ring->fbr[0] == NULL)
+ return -ENOMEM;
rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
+ if (rx_ring->fbr[1] == NULL)
+ return -ENOMEM;
/* The first thing we will do is configure the sizes of the buffer
* rings. These will change based on jumbo packet support. Larger
@@ -2289,7 +2293,7 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
for (id = 0; id < NUM_FBRS; id++) {
fbr = rx_ring->fbr[id];
- if (!fbr->ring_virtaddr)
+ if (!fbr || !fbr->ring_virtaddr)
continue;
/* First the packet memory */
@@ -3591,6 +3595,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
if (status) {
dev_err(&adapter->pdev->dev,
"et131x_tx_dma_memory_alloc FAILED\n");
+ et131x_tx_dma_memory_free(adapter);
return status;
}
/* Receive buffer memory allocation */
@@ -3598,7 +3603,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
if (status) {
dev_err(&adapter->pdev->dev,
"et131x_rx_dma_memory_alloc FAILED\n");
- et131x_tx_dma_memory_free(adapter);
+ et131x_adapter_memory_free(adapter);
return status;
}
Alan, thanks for resending this patch. But it seems you overlooked
something we discussed earlier.
On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote:
> We should check the ring allocations don't fail.
> If we get a fail we need to clean up properly. The allocator assumes the
> deallocator will be used on failure, but it isn't. Make sure the
> right deallocator is always called and add a missing check against
> fbr allocation failure.
>
> [v2]: Correct check logic
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
> drivers/staging/et131x/et131x.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index 6413500..cc600df 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>
> /* Alloc memory for the lookup table */
> rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> + if (rx_ring->fbr[0] == NULL)
> + return -ENOMEM;
> rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> + if (rx_ring->fbr[1] == NULL)
> + return -ENOMEM;
Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1]
fails ? Or we will leak memory here.
>
> /* The first thing we will do is configure the sizes of the buffer
> * rings. These will change based on jumbo packet support. Larger
> @@ -2289,7 +2293,7 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
> for (id = 0; id < NUM_FBRS; id++) {
> fbr = rx_ring->fbr[id];
>
> - if (!fbr->ring_virtaddr)
> + if (!fbr || !fbr->ring_virtaddr)
> continue;
>
> /* First the packet memory */
> @@ -3591,6 +3595,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
> if (status) {
> dev_err(&adapter->pdev->dev,
> "et131x_tx_dma_memory_alloc FAILED\n");
> + et131x_tx_dma_memory_free(adapter);
> return status;
> }
> /* Receive buffer memory allocation */
> @@ -3598,7 +3603,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
> if (status) {
> dev_err(&adapter->pdev->dev,
> "et131x_rx_dma_memory_alloc FAILED\n");
> - et131x_tx_dma_memory_free(adapter);
> + et131x_adapter_memory_free(adapter);
> return status;
> }
>
On Wed, 19 Feb 2014 09:14:19 +0800
"Zhao\, Gang" <[email protected]> wrote:
> Alan, thanks for resending this patch. But it seems you overlooked
> something we discussed earlier.
>
> On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote:
> > We should check the ring allocations don't fail.
> > If we get a fail we need to clean up properly. The allocator assumes the
> > deallocator will be used on failure, but it isn't. Make sure the
> > right deallocator is always called and add a missing check against
> > fbr allocation failure.
> >
> > [v2]: Correct check logic
> >
> > Signed-off-by: Alan Cox <[email protected]>
> > ---
> > drivers/staging/et131x/et131x.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> > index 6413500..cc600df 100644
> > --- a/drivers/staging/et131x/et131x.c
> > +++ b/drivers/staging/et131x/et131x.c
> > @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> >
> > /* Alloc memory for the lookup table */
> > rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> > + if (rx_ring->fbr[0] == NULL)
> > + return -ENOMEM;
> > rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> > + if (rx_ring->fbr[1] == NULL)
> > + return -ENOMEM;
>
> Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1]
> fails ? Or we will leak memory here.
No.. the tx_dma_memory_free and rx_dma_memory_free functions are
designed to handle incomplete set up. They are now called on incomplete
setup and will clean up all the resources.
> > @@ -3591,6 +3595,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
> > if (status) {
> > dev_err(&adapter->pdev->dev,
> > "et131x_tx_dma_memory_alloc FAILED\n");
> > + et131x_tx_dma_memory_free(adapter);
> > return status;
> > }
> > /* Receive buffer memory allocation */
> > @@ -3598,7 +3603,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
> > if (status) {
> > dev_err(&adapter->pdev->dev,
> > "et131x_rx_dma_memory_alloc FAILED\n");
> > - et131x_tx_dma_memory_free(adapter);
> > + et131x_adapter_memory_free(adapter);
> > return status;
> > }
> >
Which is what these changes are about.
Whoever wrote the allocator and cleanup methods arranged (except for the
rx_ring->fbr cases) that the free method should be called on a failure.
It looks as if somewhere along the line of the driver development whoever
wrote the higher level bits didn't understand that.
Alan
On Wed, 2014-02-19 at 19:43:15 +0800, One Thousand Gnomes wrote:
> On Wed, 19 Feb 2014 09:14:19 +0800
> "Zhao\, Gang" <[email protected]> wrote:
>
>> Alan, thanks for resending this patch. But it seems you overlooked
>> something we discussed earlier.
>>
>> On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote:
>> > We should check the ring allocations don't fail.
>> > If we get a fail we need to clean up properly. The allocator assumes the
>> > deallocator will be used on failure, but it isn't. Make sure the
>> > right deallocator is always called and add a missing check against
>> > fbr allocation failure.
>> >
>> > [v2]: Correct check logic
>> >
>> > Signed-off-by: Alan Cox <[email protected]>
>> > ---
>> > drivers/staging/et131x/et131x.c | 9 +++++++--
>> > 1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
>> > index 6413500..cc600df 100644
>> > --- a/drivers/staging/et131x/et131x.c
>> > +++ b/drivers/staging/et131x/et131x.c
>> > @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> >
>> > /* Alloc memory for the lookup table */
>> > rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> > + if (rx_ring->fbr[0] == NULL)
>> > + return -ENOMEM;
>> > rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> > + if (rx_ring->fbr[1] == NULL)
>> > + return -ENOMEM;
>>
>> Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1]
>> fails ? Or we will leak memory here.
>
> No.. the tx_dma_memory_free and rx_dma_memory_free functions are
> designed to handle incomplete set up. They are now called on incomplete
> setup and will clean up all the resources.
>
Yes, you are right. By calling {tx, rx}_dma_memory_free the memory will
be freed.
But I think a comment is needed here, to make this more clear ? Without
proper comment the above code looks a little strange to let one think
it's right. :)
>> > @@ -3591,6 +3595,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
>> > if (status) {
>> > dev_err(&adapter->pdev->dev,
>> > "et131x_tx_dma_memory_alloc FAILED\n");
>> > + et131x_tx_dma_memory_free(adapter);
>> > return status;
>> > }
>> > /* Receive buffer memory allocation */
>> > @@ -3598,7 +3603,7 @@ static int et131x_adapter_memory_alloc(struct et131x_adapter *adapter)
>> > if (status) {
>> > dev_err(&adapter->pdev->dev,
>> > "et131x_rx_dma_memory_alloc FAILED\n");
>> > - et131x_tx_dma_memory_free(adapter);
>> > + et131x_adapter_memory_free(adapter);
>> > return status;
>> > }
>> >
>
> Which is what these changes are about.
>
> Whoever wrote the allocator and cleanup methods arranged (except for the
> rx_ring->fbr cases) that the free method should be called on a failure.
> It looks as if somewhere along the line of the driver development whoever
> wrote the higher level bits didn't understand that.
>
> Alan
On Thu, Feb 20, 2014 at 11:03:45AM +0800, Zhao, Gang wrote:
> On Wed, 2014-02-19 at 19:43:15 +0800, One Thousand Gnomes wrote:
> > On Wed, 19 Feb 2014 09:14:19 +0800
> > "Zhao\, Gang" <[email protected]> wrote:
> >
> >> Alan, thanks for resending this patch. But it seems you overlooked
> >> something we discussed earlier.
> >>
> >> On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote:
> >> > We should check the ring allocations don't fail.
> >> > If we get a fail we need to clean up properly. The allocator assumes the
> >> > deallocator will be used on failure, but it isn't. Make sure the
> >> > right deallocator is always called and add a missing check against
> >> > fbr allocation failure.
> >> >
> >> > [v2]: Correct check logic
> >> >
> >> > Signed-off-by: Alan Cox <[email protected]>
> >> > ---
> >> > drivers/staging/et131x/et131x.c | 9 +++++++--
> >> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> >> > index 6413500..cc600df 100644
> >> > --- a/drivers/staging/et131x/et131x.c
> >> > +++ b/drivers/staging/et131x/et131x.c
> >> > @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> >> >
> >> > /* Alloc memory for the lookup table */
> >> > rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> >> > + if (rx_ring->fbr[0] == NULL)
> >> > + return -ENOMEM;
> >> > rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> >> > + if (rx_ring->fbr[1] == NULL)
> >> > + return -ENOMEM;
> >>
> >> Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1]
> >> fails ? Or we will leak memory here.
> >
> > No.. the tx_dma_memory_free and rx_dma_memory_free functions are
> > designed to handle incomplete set up. They are now called on incomplete
> > setup and will clean up all the resources.
> >
>
> Yes, you are right. By calling {tx, rx}_dma_memory_free the memory will
> be freed.
>
> But I think a comment is needed here, to make this more clear ? Without
> proper comment the above code looks a little strange to let one think
> it's right. :)
No. We don't need a comment. If people start adding kfree() calls
all over the place without thinking then we are already screwed and no
comment is going to help us.
regards,
dan carpenter
On Thu, 2014-02-20 at 17:03:39 +0800, Dan Carpenter wrote:
> On Thu, Feb 20, 2014 at 11:03:45AM +0800, Zhao, Gang wrote:
>> On Wed, 2014-02-19 at 19:43:15 +0800, One Thousand Gnomes wrote:
>> > On Wed, 19 Feb 2014 09:14:19 +0800
>> > "Zhao\, Gang" <[email protected]> wrote:
>> >
>> >> Alan, thanks for resending this patch. But it seems you overlooked
>> >> something we discussed earlier.
>> >>
>> >> On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote:
>> >> > We should check the ring allocations don't fail.
>> >> > If we get a fail we need to clean up properly. The allocator assumes the
>> >> > deallocator will be used on failure, but it isn't. Make sure the
>> >> > right deallocator is always called and add a missing check against
>> >> > fbr allocation failure.
>> >> >
>> >> > [v2]: Correct check logic
>> >> >
>> >> > Signed-off-by: Alan Cox <[email protected]>
>> >> > ---
>> >> > drivers/staging/et131x/et131x.c | 9 +++++++--
>> >> > 1 file changed, 7 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
>> >> > index 6413500..cc600df 100644
>> >> > --- a/drivers/staging/et131x/et131x.c
>> >> > +++ b/drivers/staging/et131x/et131x.c
>> >> > @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> >> >
>> >> > /* Alloc memory for the lookup table */
>> >> > rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> >> > + if (rx_ring->fbr[0] == NULL)
>> >> > + return -ENOMEM;
>> >> > rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> >> > + if (rx_ring->fbr[1] == NULL)
>> >> > + return -ENOMEM;
>> >>
>> >> Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1]
>> >> fails ? Or we will leak memory here.
>> >
>> > No.. the tx_dma_memory_free and rx_dma_memory_free functions are
>> > designed to handle incomplete set up. They are now called on incomplete
>> > setup and will clean up all the resources.
>> >
>>
>> Yes, you are right. By calling {tx, rx}_dma_memory_free the memory will
>> be freed.
>>
>> But I think a comment is needed here, to make this more clear ? Without
>> proper comment the above code looks a little strange to let one think
>> it's right. :)
>
> No. We don't need a comment. If people start adding kfree() calls
> all over the place without thinking then we are already screwed and no
> comment is going to help us.
Hi, I thought this a little more.
AFAIK, most functions deal with this "fail in the middle" allocation
failure themselves. Honestly, relying on the caller to handle this type
of error seems a bad idea to me.
Code reviewer has to check *every* caller of this function to make sure
whether rx_ring->fbr[0] is leaked or not when allocation of
rx_ring->fbr[1] fails.(By examing if the caller called the correct
freeing function when this function returns error) This is just a waste
of time. By freeing rx_ring->fbr[0] in this function the above type of
memory leak can't be happen at the beginning.
So now my suggestion is freeing rx_ring->fbr[0] *and* set the pointer
rx_ring->fbr[0] to NULL when allocation of rx_ring->fbr[1] fails *in*
this function. The freeing function which can handle "fail in the
middle" allocation failure surely can handle this situation correctly,
isn't it ?
>
> regards,
> dan carpenter