| 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 |
| 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 |
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;
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;
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;
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; >
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!
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 --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;