On Mon, Jul 04, 2011 at 05:11:35PM -0700, Matthew Dempsky wrote:
> A bunch of disk drivers currently check whether a buf request is
> sector-aligned (e.g., vnd(4)) or that a request is for a multiple of
> secsize (e.g., sd(4)), but none of them check both.
>
> Instead of doing this in each disk driver, the logical place to
> validate this is in bounds_check_with_label().
>
> While here, I rewrote bounds_check_with_label() to be a bit easier to
> read and be safer with integer arithmetic to avoid overflows (though I
> don't think in practice this is actually a risk).
>
> Once this goes in, I'll start updating the xxstrategy() methods to
> remove these redundant checks.
>
> ok?
>
>
> Index: subr_disk.c
> ===================================================================
> RCS file: /home/mdempsky/anoncvs/cvs/src/sys/kern/subr_disk.c,v
> retrieving revision 1.127
> diff -u -p -r1.127 subr_disk.c
> --- subr_disk.c 30 Jun 2011 16:28:05 -0000 1.127
> +++ subr_disk.c 4 Jul 2011 23:59:39 -0000
> @@ -687,25 +687,30 @@ int
> bounds_check_with_label(struct buf *bp, struct disklabel *lp)
> {
> struct partition *p = &lp->d_partitions[DISKPART(bp->b_dev)];
> - daddr64_t sz = howmany(bp->b_bcount, DEV_BSIZE);
> + daddr64_t partblocks, sz;
>
> - /* Avoid division by zero, negative offsets and negative sizes. */
> - if (lp->d_secpercyl == 0 || bp->b_blkno < 0 || sz < 0)
> + /* Avoid division by zero, negative offsets, and negative sizes. */
> + if (lp->d_secpercyl == 0 || bp->b_blkno < 0 || bp->b_bcount < 0)
> goto bad;
>
> - /* beyond partition? */
> - if (bp->b_blkno + sz > DL_SECTOBLK(lp, DL_GETPSIZE(p))) {
> - sz = DL_SECTOBLK(lp, DL_GETPSIZE(p)) - bp->b_blkno;
> - if (sz == 0) {
> - /* If exactly at end of disk, return EOF. */
> - bp->b_resid = bp->b_bcount;
> - return (-1);
> - }
> - if (sz < 0)
> - /* If past end of disk, return EINVAL. */
> - goto bad;
> + /* Ensure transfer is a whole number of aligned sectors. */
> + if ((bp->b_blkno % DL_BLKSPERSEC(lp)) != 0 ||
> + (bp->b_bcount % lp->d_secsize) != 0)
> + goto bad;
> +
> + /* Ensure transfer starts within partition boundary. */
> + partblocks = DL_SECTOBLK(lp, DL_GETPSIZE(p));
> + if (bp->b_blkno > partblocks)
> + goto bad;
> +
> + /* If exactly at end of partition or 0-byte request, return EOF. */
> + if (bp->b_blkno == partblocks || bp->b_bcount == 0)
> + goto done;
>
> - /* Otherwise, truncate request. */
> + /* Truncate request if it exceeds past the end of the partition. */
> + sz = bp->b_bcount >> DEV_BSHIFT;
> + if (sz > partblocks - bp->b_blkno) {
> + sz = partblocks - bp->b_blkno;
> bp->b_bcount = sz << DEV_BSHIFT;
> }
>
> @@ -714,9 +719,11 @@ bounds_check_with_label(struct buf *bp,
> DL_SECTOBLK(lp, lp->d_secpercyl);
> return (1);
>
> -bad:
> + bad:
> bp->b_error = EINVAL;
> bp->b_flags |= B_ERROR;
> + done:
> + bp->b_resid = bp->b_bcount;
> return (-1);
> }
>
I like this a *lot*. My only question is why the whitespace before bad: and
done:? ok krw@
.... Ken
