From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Demi Marie Obenour <demi@invisiblethingslab.com>
Cc: "Jens Axboe" <axboe@kernel.dk>,
"Alasdair Kergon" <agk@redhat.com>,
"Mike Snitzer" <snitzer@kernel.org>,
dm-devel@redhat.com,
"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 15/16] xen-blkback: Minor cleanups
Date: Tue, 6 Jun 2023 10:36:46 +0200 [thread overview]
Message-ID: <ZH7wHiiF/OVu1W8D@Air-de-Roger> (raw)
In-Reply-To: <20230530203116.2008-16-demi@invisiblethingslab.com>
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@invisiblethingslab.com>
> ---
> 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.
next prev parent reply other threads:[~2023-06-06 8:37 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-30 20:31 [PATCH v2 00/16] Diskseq support in loop, device-mapper, and blkback Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 01/16] device-mapper: Check that target specs are sufficiently aligned Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 02/16] device-mapper: Avoid pointer arithmetic overflow Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 03/16] device-mapper: do not allow targets to overlap 'struct dm_ioctl' Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 04/16] device-mapper: Better error message for too-short target spec Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 05/16] device-mapper: Target parameters must not overlap next " Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 06/16] device-mapper: Avoid double-fetch of version Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 07/16] device-mapper: Allow userspace to opt-in to strict parameter checks Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 08/16] device-mapper: Allow userspace to provide expected diskseq Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 09/16] device-mapper: Allow userspace to suppress uevent generation Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 10/16] device-mapper: Refuse to create device named "control" Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 11/16] device-mapper: "." and ".." are not valid symlink names Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 12/16] device-mapper: inform caller about already-existing device Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 13/16] xen-blkback: Implement diskseq checks Demi Marie Obenour
2023-06-06 8:25 ` Roger Pau Monné
2023-06-06 17:01 ` Demi Marie Obenour
2023-06-07 8:20 ` Roger Pau Monné
2023-06-07 16:14 ` Demi Marie Obenour
2023-06-08 8:29 ` Roger Pau Monné
2023-06-08 15:33 ` Demi Marie Obenour
2023-06-09 15:13 ` Roger Pau Monné
2023-06-09 16:55 ` Demi Marie Obenour
2023-06-12 8:09 ` Roger Pau Monné
2023-06-21 1:14 ` Demi Marie Obenour
2023-06-21 10:07 ` Roger Pau Monné
2023-05-30 20:31 ` [PATCH v2 14/16] block, loop: Increment diskseq when releasing a loop device Demi Marie Obenour
2023-05-30 20:31 ` [PATCH v2 15/16] xen-blkback: Minor cleanups Demi Marie Obenour
2023-06-06 8:36 ` Roger Pau Monné [this message]
2023-05-30 20:31 ` [PATCH v2 16/16] xen-blkback: Inform userspace that device has been opened Demi Marie Obenour
2023-06-06 9:15 ` Roger Pau Monné
2023-06-06 17:31 ` Demi Marie Obenour
2023-06-07 8:44 ` Roger Pau Monné
2023-06-07 16:29 ` Demi Marie Obenour
2023-06-08 9:11 ` Roger Pau Monné
2023-06-08 15:23 ` Demi Marie Obenour
2023-06-08 10:08 ` Roger Pau Monné
2023-06-08 15:24 ` Demi Marie Obenour
2023-05-31 13:06 ` [dm-devel] [PATCH v2 00/16] Diskseq support in loop, device-mapper, and blkback Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZH7wHiiF/OVu1W8D@Air-de-Roger \
--to=roger.pau@citrix.com \
--cc=agk@redhat.com \
--cc=axboe@kernel.dk \
--cc=demi@invisiblethingslab.com \
--cc=dm-devel@redhat.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marmarek@invisiblethingslab.com \
--cc=snitzer@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).