Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

[Corey Minyard]

Patch is queued for 3.20.  Thanks.

-corey

On 01/19/2015 09:14 PM, Nicholas Krause wrote:
> This adds a loop through the elements in the linked list, recv_msgs using
> list_for_entry_safe in order to free messages in this list.  In addition
> we are using the safe version of this marco in order to prevent use after
> bugs related to deleting the element we are on currently by holding a
> pointer to the next element after the current one we are on and freeing
> with the function, ipmi_free_recv_msg internally in this loop.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/char/ipmi/ipmi_devintf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> index ec318bf..d2af38a 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -157,14 +157,15 @@ static int ipmi_release(struct inode *inode, struct file *file)
>  {
>  	struct ipmi_file_private *priv = file->private_data;
>  	int                      rv;
> +	struct  ipmi_recv_msg *msg, *next;
>  
>  	rv = ipmi_destroy_user(priv->user);
>  	if (rv)
>  		return rv;
>  
> -	/* FIXME - free the messages in the list. */
> -	kfree(priv);
> -
> +	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link) {
> +		ipmi_free_recv_msg(msg);
> +	}
>  	return 0;
>  }
>  

Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

[Sasha Levin]

On 01/22/2015 08:05 AM, Corey Minyard wrote:
> Patch is queued for 3.20.  Thanks.

That patch is horribly broken.


Thanks,
Sasha

> -corey
> 
> On 01/19/2015 09:14 PM, Nicholas Krause wrote:
>> This adds a loop through the elements in the linked list, recv_msgs using
>> list_for_entry_safe in order to free messages in this list.  In addition
>> we are using the safe version of this marco in order to prevent use after
>> bugs related to deleting the element we are on currently by holding a
>> pointer to the next element after the current one we are on and freeing
>> with the function, ipmi_free_recv_msg internally in this loop.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/char/ipmi/ipmi_devintf.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
>> index ec318bf..d2af38a 100644
>> --- a/drivers/char/ipmi/ipmi_devintf.c
>> +++ b/drivers/char/ipmi/ipmi_devintf.c
>> @@ -157,14 +157,15 @@ static int ipmi_release(struct inode *inode, struct file *file)
>>  {
>>  	struct ipmi_file_private *priv = file->private_data;
>>  	int                      rv;
>> +	struct  ipmi_recv_msg *msg, *next;
>>  
>>  	rv = ipmi_destroy_user(priv->user);
>>  	if (rv)
>>  		return rv;
>>  
>> -	/* FIXME - free the messages in the list. */
>> -	kfree(priv);
>> -
>> +	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link) {
>> +		ipmi_free_recv_msg(msg);
>> +	}
>>  	return 0;
>>  }
>>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

[Sasha Levin]

On 01/22/2015 08:05 AM, Corey Minyard wrote:
> Patch is queued for 3.20.  Thanks.

That patch is horribly broken.


Thanks,
Sasha

> -corey
> 
> On 01/19/2015 09:14 PM, Nicholas Krause wrote:
>> This adds a loop through the elements in the linked list, recv_msgs using
>> list_for_entry_safe in order to free messages in this list.  In addition
>> we are using the safe version of this marco in order to prevent use after
>> bugs related to deleting the element we are on currently by holding a
>> pointer to the next element after the current one we are on and freeing
>> with the function, ipmi_free_recv_msg internally in this loop.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  drivers/char/ipmi/ipmi_devintf.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
>> index ec318bf..d2af38a 100644
>> --- a/drivers/char/ipmi/ipmi_devintf.c
>> +++ b/drivers/char/ipmi/ipmi_devintf.c
>> @@ -157,14 +157,15 @@ static int ipmi_release(struct inode *inode, struct file *file)
>>  {
>>  	struct ipmi_file_private *priv = file->private_data;
>>  	int                      rv;
>> +	struct  ipmi_recv_msg *msg, *next;
>>  
>>  	rv = ipmi_destroy_user(priv->user);
>>  	if (rv)
>>  		return rv;
>>  
>> -	/* FIXME - free the messages in the list. */
>> -	kfree(priv);
>> -
>> +	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link) {
>> +		ipmi_free_recv_msg(msg);
>> +	}
>>  	return 0;
>>  }
>>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

[Corey Minyard]

On 01/23/2015 10:35 AM, nick wrote:
> Sasha,
> How is this patch broken? It builds and clearly frees 
> the list entries. Please explain how to me.
> Nick

Oh, it does not free priv.  That code should not have been removed.

-corey

> On 2015-01-22 11:50 PM, Sasha Levin wrote:
>> On 01/22/2015 08:05 AM, Corey Minyard wrote:
>>> Patch is queued for 3.20.  Thanks.
>> That patch is horribly broken.
>>
>>
>> Thanks,
>> Sasha
>>
>>> -corey
>>>
>>> On 01/19/2015 09:14 PM, Nicholas Krause wrote:
>>>> This adds a loop through the elements in the linked list, recv_msgs using
>>>> list_for_entry_safe in order to free messages in this list.  In addition
>>>> we are using the safe version of this marco in order to prevent use after
>>>> bugs related to deleting the element we are on currently by holding a
>>>> pointer to the next element after the current one we are on and freeing
>>>> with the function, ipmi_free_recv_msg internally in this loop.
>>>>
>>>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>>>> ---
>>>>  drivers/char/ipmi/ipmi_devintf.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
>>>> index ec318bf..d2af38a 100644
>>>> --- a/drivers/char/ipmi/ipmi_devintf.c
>>>> +++ b/drivers/char/ipmi/ipmi_devintf.c
>>>> @@ -157,14 +157,15 @@ static int ipmi_release(struct inode *inode, struct file *file)
>>>>  {
>>>>  	struct ipmi_file_private *priv = file->private_data;
>>>>  	int                      rv;
>>>> +	struct  ipmi_recv_msg *msg, *next;
>>>>  
>>>>  	rv = ipmi_destroy_user(priv->user);
>>>>  	if (rv)
>>>>  		return rv;
>>>>  
>>>> -	/* FIXME - free the messages in the list. */
>>>> -	kfree(priv);
>>>> -
>>>> +	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link) {
>>>> +		ipmi_free_recv_msg(msg);
>>>> +	}
>>>>  	return 0;
>>>>  }
>>>>  
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>

Re: [PATCH RESEND] char:ipmi: Free ipmi_recv_msg messages from the linked list,recv_msgs for the function,ipmi_release in the file,ipmi_devintf.c

[Corey Minyard]

I've been debating what to do with this, but the patch makes a good
change, so
I'll go ahead with it.  I am going to remove the unnecessary braces.

In the future, you really need to be able to test the changes you make. 
So you really
need to work in an area that you can test in.  If you are really
interested in learning,
you have a ways to go, I think, and the kernel may not be the best place
to do that
learning.

-corey

On 01/30/2015 11:17 PM, Nicholas Krause wrote:
> This adds a loop through the elements in the linked list, recv_msgs using
> list_for_entry_safe in order to free messages in this list.  In addition
> we are using the safe version of this marco in order to prevent use after
> bugs related to deleting the element we are on currently by holding a
> pointer to the next element after the current one we are on and freeing
> with the function, ipmi_free_recv_msg internally in this loop.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/char/ipmi/ipmi_devintf.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> index ec318bf..235a1fa 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -157,12 +157,17 @@ static int ipmi_release(struct inode *inode, struct file *file)
>  {
>  	struct ipmi_file_private *priv = file->private_data;
>  	int                      rv;
> +	struct  ipmi_recv_msg *msg, *next;
>  
>  	rv = ipmi_destroy_user(priv->user);
>  	if (rv)
>  		return rv;
>  
> -	/* FIXME - free the messages in the list. */
> +	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link) {
> +		ipmi_free_recv_msg(msg);
> +	}
> +
> +
>  	kfree(priv);
>  
>  	return 0;