diff mbox series

9p/xen: Fix end of loop tests for list_for_each_entry

Message ID 20210725175103.56731-1-harshvardhan.jha@oracle.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series 9p/xen: Fix end of loop tests for list_for_each_entry | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Harshvardhan Jha July 25, 2021, 5:51 p.m. UTC
The list_for_each_entry() iterator, "priv" in this code, can never be
NULL so the warning would never be printed.

Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
---
>From static analysis.  Not tested.
---
 net/9p/trans_xen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dominique Martinet July 25, 2021, 8:46 p.m. UTC | #1
Harshvardhan Jha wrote on Sun, Jul 25, 2021 at 11:21:03PM +0530:
> The list_for_each_entry() iterator, "priv" in this code, can never be
> NULL so the warning would never be printed.

hm? priv won't be NULL but priv->client won't be client, so it will
return -EINVAL alright in practice?

This does fix an invalid read after the list head, so there's a real
bug, but the commit message needs fixing.

> 
> Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
> ---
> From static analysis.  Not tested.

+Stefano in To - I also can't test xen right now :/
This looks functional to me but if you have a bit of time to spare just
a mount test can't hurt.

> ---
>  net/9p/trans_xen.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index f4fea28e05da..3ec1a51a6944 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -138,7 +138,7 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>  
>  static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>  {
> -	struct xen_9pfs_front_priv *priv = NULL;
> +	struct xen_9pfs_front_priv *priv;
>  	RING_IDX cons, prod, masked_cons, masked_prod;
>  	unsigned long flags;
>  	u32 size = p9_req->tc.size;
> @@ -151,7 +151,7 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>  			break;
>  	}
>  	read_unlock(&xen_9pfs_lock);
> -	if (!priv || priv->client != client)
> +	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
>  		return -EINVAL;
>  
>  	num = p9_req->tc.tag % priv->num_rings;
Stefano Stabellini July 26, 2021, 9:30 p.m. UTC | #2
On Mon, 26 Jul 2021, asmadeus@codewreck.org wrote:
> Harshvardhan Jha wrote on Sun, Jul 25, 2021 at 11:21:03PM +0530:
> > The list_for_each_entry() iterator, "priv" in this code, can never be
> > NULL so the warning would never be printed.
> 
> hm? priv won't be NULL but priv->client won't be client, so it will
> return -EINVAL alright in practice?
> 
> This does fix an invalid read after the list head, so there's a real
> bug, but the commit message needs fixing.

Agreed


> > Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
> > ---
> > From static analysis.  Not tested.
> 
> +Stefano in To - I also can't test xen right now :/
> This looks functional to me but if you have a bit of time to spare just
> a mount test can't hurt.

Yes, I did test it successfully. Aside from the commit messaged to be
reworded:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> >  net/9p/trans_xen.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index f4fea28e05da..3ec1a51a6944 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> > @@ -138,7 +138,7 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >  
> >  static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >  {
> > -	struct xen_9pfs_front_priv *priv = NULL;
> > +	struct xen_9pfs_front_priv *priv;
> >  	RING_IDX cons, prod, masked_cons, masked_prod;
> >  	unsigned long flags;
> >  	u32 size = p9_req->tc.size;
> > @@ -151,7 +151,7 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >  			break;
> >  	}
> >  	read_unlock(&xen_9pfs_lock);
> > -	if (!priv || priv->client != client)
> > +	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
> >  		return -EINVAL;
> >  
> >  	num = p9_req->tc.tag % priv->num_rings;
Harshvardhan Jha July 26, 2021, 10:23 p.m. UTC | #3
On 27/07/21 3:00 am, Stefano Stabellini wrote:
> On Mon, 26 Jul 2021, asmadeus@codewreck.org wrote:
>> Harshvardhan Jha wrote on Sun, Jul 25, 2021 at 11:21:03PM +0530:
>>> The list_for_each_entry() iterator, "priv" in this code, can never be
>>> NULL so the warning would never be printed.
>>
>> hm? priv won't be NULL but priv->client won't be client, so it will
>> return -EINVAL alright in practice?
>>
>> This does fix an invalid read after the list head, so there's a real
>> bug, but the commit message needs fixing.
> 
> Agreed
> 
> 
>>> Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
>>> ---
>>>  From static analysis.  Not tested.
>>
>> +Stefano in To - I also can't test xen right now :/
>> This looks functional to me but if you have a bit of time to spare just
>> a mount test can't hurt.
> 
> Yes, I did test it successfully. Aside from the commit messaged to be
> reworded:
How's this?
===========================BEGIN========================================
9p/xen: Fix end of loop tests for list_for_each_entry

This patch addresses the following problems:
  - priv can never be NULL, so this part of the check is useless
  - if the loop ran through the whole list, priv->client is invalid and
it is more appropriate and sufficient to check for the end of
list_for_each_entry loop condition.

Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
---
 From static analysis. Not tested.
===========================END==========================================
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Tested-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>>> ---
>>>   net/9p/trans_xen.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
>>> index f4fea28e05da..3ec1a51a6944 100644
>>> --- a/net/9p/trans_xen.c
>>> +++ b/net/9p/trans_xen.c
>>> @@ -138,7 +138,7 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>>>   
>>>   static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>>   {
>>> -	struct xen_9pfs_front_priv *priv = NULL;
>>> +	struct xen_9pfs_front_priv *priv;
>>>   	RING_IDX cons, prod, masked_cons, masked_prod;
>>>   	unsigned long flags;
>>>   	u32 size = p9_req->tc.size;
>>> @@ -151,7 +151,7 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>>   			break;
>>>   	}
>>>   	read_unlock(&xen_9pfs_lock);
>>> -	if (!priv || priv->client != client)
>>> +	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
>>>   		return -EINVAL;
>>>   
>>>   	num = p9_req->tc.tag % priv->num_rings;
Stefano Stabellini July 26, 2021, 11:54 p.m. UTC | #4
On Tue, 27 Jul 2021, Harshvardhan Jha wrote:
> On 27/07/21 3:00 am, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2021, asmadeus@codewreck.org wrote:
> > > Harshvardhan Jha wrote on Sun, Jul 25, 2021 at 11:21:03PM +0530:
> > > > The list_for_each_entry() iterator, "priv" in this code, can never be
> > > > NULL so the warning would never be printed.
> > > 
> > > hm? priv won't be NULL but priv->client won't be client, so it will
> > > return -EINVAL alright in practice?
> > > 
> > > This does fix an invalid read after the list head, so there's a real
> > > bug, but the commit message needs fixing.
> > 
> > Agreed
> > 
> > 
> > > > Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
> > > > ---
> > > >  From static analysis.  Not tested.
> > > 
> > > +Stefano in To - I also can't test xen right now :/
> > > This looks functional to me but if you have a bit of time to spare just
> > > a mount test can't hurt.
> > 
> > Yes, I did test it successfully. Aside from the commit messaged to be
> > reworded:
> How's this?
> ===========================BEGIN========================================
> 9p/xen: Fix end of loop tests for list_for_each_entry
> 
> This patch addresses the following problems:
>  - priv can never be NULL, so this part of the check is useless
>  - if the loop ran through the whole list, priv->client is invalid and
> it is more appropriate and sufficient to check for the end of
> list_for_each_entry loop condition.
> 
> Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>

That's fine


> ---
> From static analysis. Not tested.
> ===========================END==========================================
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > Tested-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > 
> > > > ---
> > > >   net/9p/trans_xen.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > > > index f4fea28e05da..3ec1a51a6944 100644
> > > > --- a/net/9p/trans_xen.c
> > > > +++ b/net/9p/trans_xen.c
> > > > @@ -138,7 +138,7 @@ static bool p9_xen_write_todo(struct
> > > > xen_9pfs_dataring *ring, RING_IDX size)
> > > >     static int p9_xen_request(struct p9_client *client, struct p9_req_t
> > > > *p9_req)
> > > >   {
> > > > -	struct xen_9pfs_front_priv *priv = NULL;
> > > > +	struct xen_9pfs_front_priv *priv;
> > > >   	RING_IDX cons, prod, masked_cons, masked_prod;
> > > >   	unsigned long flags;
> > > >   	u32 size = p9_req->tc.size;
> > > > @@ -151,7 +151,7 @@ static int p9_xen_request(struct p9_client *client,
> > > > struct p9_req_t *p9_req)
> > > >   			break;
> > > >   	}
> > > >   	read_unlock(&xen_9pfs_lock);
> > > > -	if (!priv || priv->client != client)
> > > > +	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
> > > >   		return -EINVAL;
> > > >     	num = p9_req->tc.tag % priv->num_rings;
>
Dominique Martinet July 26, 2021, 11:59 p.m. UTC | #5
Stefano Stabellini wrote on Mon, Jul 26, 2021 at 04:54:29PM -0700:
> > > Yes, I did test it successfully. Aside from the commit messaged to be
> > > reworded:
> > How's this?
> > ===========================BEGIN========================================
> > 9p/xen: Fix end of loop tests for list_for_each_entry
> > 
> > This patch addresses the following problems:
> >  - priv can never be NULL, so this part of the check is useless
> >  - if the loop ran through the whole list, priv->client is invalid and
> > it is more appropriate and sufficient to check for the end of
> > list_for_each_entry loop condition.
> > 
> > Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>

Will take the patch with this text as commit message later tonight


> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > Tested-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks for the test!
Harshvardhan Jha July 27, 2021, 12:01 a.m. UTC | #6
On 27/07/21 5:29 am, asmadeus@codewreck.org wrote:
> Stefano Stabellini wrote on Mon, Jul 26, 2021 at 04:54:29PM -0700:
>>>> Yes, I did test it successfully. Aside from the commit messaged to be
>>>> reworded:
>>> How's this?
>>> ===========================BEGIN========================================
>>> 9p/xen: Fix end of loop tests for list_for_each_entry
>>>
>>> This patch addresses the following problems:
>>>   - priv can never be NULL, so this part of the check is useless
>>>   - if the loop ran through the whole list, priv->client is invalid and
>>> it is more appropriate and sufficient to check for the end of
>>> list_for_each_entry loop condition.
>>>
>>> Signed-off-by: Harshvardhan Jha <harshvardhan.jha@oracle.com>
> 
> Will take the patch with this text as commit message later tonight
If you want I can resend the patch with this commit message
> 
> 
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>> Tested-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Thanks for the test!
>
diff mbox series

Patch

diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index f4fea28e05da..3ec1a51a6944 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -138,7 +138,7 @@  static bool p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
 
 static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
 {
-	struct xen_9pfs_front_priv *priv = NULL;
+	struct xen_9pfs_front_priv *priv;
 	RING_IDX cons, prod, masked_cons, masked_prod;
 	unsigned long flags;
 	u32 size = p9_req->tc.size;
@@ -151,7 +151,7 @@  static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
 			break;
 	}
 	read_unlock(&xen_9pfs_lock);
-	if (!priv || priv->client != client)
+	if (list_entry_is_head(priv, &xen_9pfs_devs, list))
 		return -EINVAL;
 
 	num = p9_req->tc.tag % priv->num_rings;