Re: [PATCH v2 15/16] xen-blkback: Minor cleanups

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 30, 2023 at 04:31:15PM -0400, Demi Marie Obenour wrote:
> This adds a couple of BUILD_BUG_ON()s and moves some arithmetic after
> the validation code that checks the arithmetic’s preconditions.  The
> previous code was correct but could potentially trip sanitizers that
> check for unsigned integer wraparound.
> 
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/block/xen-blkback/blkback.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index 
> c362f4ad80ab07bfb58caff0ed7da37dc1484fc5..ac760a08d559085ab875784f1c58cdf2ead95a43
>  100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1342,6 +1342,8 @@ static int dispatch_rw_block_io(struct xen_blkif_ring 
> *ring,
>       nseg = req->operation == BLKIF_OP_INDIRECT ?
>              req->u.indirect.nr_segments : req->u.rw.nr_segments;
>  
> +     BUILD_BUG_ON(offsetof(struct blkif_request, u.rw.id) != 8);
> +     BUILD_BUG_ON(offsetof(struct blkif_request, u.indirect.id) != 8);

Won't it be clearer as:

offsetof(struct blkif_request, u.rw.id) != offsetof(struct blkif_request, 
u.indirect.id)

We don't really care about the specific offset value, but both layouts
must match.

Also, you likely want to check for all requests types, not just rw and
indirect (discard and other).

>       if (unlikely(nseg == 0 && operation_flags != REQ_PREFLUSH) ||
>           unlikely((req->operation != BLKIF_OP_INDIRECT) &&
>                    (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) ||
> @@ -1365,13 +1367,13 @@ static int dispatch_rw_block_io(struct xen_blkif_ring 
> *ring,
>               preq.sector_number     = req->u.rw.sector_number;
>               for (i = 0; i < nseg; i++) {
>                       pages[i]->gref = req->u.rw.seg[i].gref;
> -                     seg[i].nsec = req->u.rw.seg[i].last_sect -
> -                             req->u.rw.seg[i].first_sect + 1;
> -                     seg[i].offset = (req->u.rw.seg[i].first_sect << 9);
>                       if ((req->u.rw.seg[i].last_sect >= (XEN_PAGE_SIZE >> 
> 9)) ||
>                           (req->u.rw.seg[i].last_sect <
>                            req->u.rw.seg[i].first_sect))
>                               goto fail_response;
> +                     seg[i].nsec = req->u.rw.seg[i].last_sect -
> +                             req->u.rw.seg[i].first_sect + 1;
> +                     seg[i].offset = (req->u.rw.seg[i].first_sect << 9);

Parentheses here are unneeded.  If we do that move, we might as well
move the assignation of pages[i]->gref as well, just to avoid
assigning to gref to take the failure path.

I do think however wraparound is not an issue here, as we will discard
the result.

Thanks, Roger.