Re: kern/46494 (cgd on 4K sector disk)

[Jan Danielsson <jan.m.danielsson%gmail.com@localhost>]

On 05/31/12 13:23, Roland C. Dowdeswell wrote:
> On Thu, May 31, 2012 at 01:08:44PM +0200, Jan Danielsson wrote:
>>    For completeness, "gpt create cgd3" returns:
>>
>>    gpt: unable to open device 'rcgd3d': Invalid argument
> 
> I've checked in a bit of code recently that should make the gpts
> work on cgd.  Please upgrade to a quite recent kernel to get that
> working.

   Thanks; switching to a -current kernel got GPT's working in the cgd
device. (What are the chances for a pull-up on this for netbsd-6?)

[---]
> We should use sector size from struct dk_softc, not duplicate it here.

   Done.

> There's a reference to DEV_BSIZE in dksubr.c that likely needs to be
> fixed as well.

   Done; but my change feels too simplistic. Is the case of
lp->d_secsize >= pdg->pdg_secsize handled somewhere, for instance?

   These changes still cause "newfs -S /dev/rdk1" to return:
   wtfs: write error for sector 183143623: Invalid argument

   Though I only updated kernel, not user land. Time to start at the
other end and see what newfs is doing.

>> @@ -406,7 +406,7 @@
>>                      return -1;
>>              }
>>              cgd_cipher(cs, newaddr, addr, bp->b_bcount, bn,
>> -                DEV_BSIZE, CGD_CIPHER_ENCRYPT);
>> +                cs->sc_secsize, CGD_CIPHER_ENCRYPT);
>>      }
> 
> This one is interesting.  We probably do not want to use the physical
> disk sector size when we're enrypting the data in the general case
> as this would mean that if you change the sector size of a disk
> image, dd a cgd between disks with different sector sizes, etc. it
> would fail to decrypt properly (using some cipher modes, currently,
> well, all of them.)
> 
> To generically solve this problem, we should likely do something like
> 
> #define CGD_BSIZE     512
> 
> at the top and use that.  We could make it configurable but this
> is actually problematic as if CGD is encrypting data in 4K chunks
> and writing to a disk with 512 byte sectors, you lose write atomicity
> which would lead to consistency issues on crashes.  So, at the very
> least, if we make it configurable then we need to enforce the
> constraint that the sector size of the underlying disk >= the
> encryption sector size.

   When you say "configurable"; are you thinking in terms of it being an
option in the kernel configuration (a global "(non)portable cgd" option
for the entire system), or making it configurable per device?

   My gut feeling is that the latter option would either be a hassle for
the user, or require a cgd parameter storage area -- a thought I dislike
strongly.

   The idea of having the ability of making cgd'd images more "portable"
is highly appealing to me, so I'll take a jab at that too. Though my
first goal is simply to get a filesystem working in a wedge in a cgd in
a wedge, with a "raw" sector size of 4K.

-- 
Kind regards,
Jan Danielsson

Index: cgd.c
===================================================================
RCS file: /cvsroot/src/sys/dev/cgd.c,v
retrieving revision 1.77
diff -u -r1.77 cgd.c
--- cgd.c       25 May 2012 10:53:46 -0000      1.77
+++ cgd.c       2 Jun 2012 14:26:12 -0000
@@ -291,6 +291,7 @@
 cgdstrategy(struct buf *bp)
 {
        struct  cgd_softc *cs = getcgd_softc(bp->b_dev);
+       struct  dk_geom *pdg = &cs->sc_dksc.sc_geom;
 
        DPRINTF_FOLLOW(("cgdstrategy(%p): b_bcount = %ld\n", bp,
            (long)bp->b_bcount));
@@ -301,7 +302,7 @@
         * buffers to be aligned to 32-bit boundaries.
         */
        if (bp->b_blkno < 0 ||
-           (bp->b_bcount % DEV_BSIZE) != 0 ||
+           (bp->b_bcount % pdg->pdg_secsize) != 0 ||
            ((uintptr_t)bp->b_data & 3) != 0) {
                bp->b_error = EINVAL;
                bp->b_resid = bp->b_bcount;
@@ -370,6 +371,7 @@
 cgdstart(struct dk_softc *dksc, struct buf *bp)
 {
        struct  cgd_softc *cs = (struct cgd_softc *)dksc;
+       struct  dk_geom *pdg = &cs->sc_dksc.sc_geom;
        struct  buf *nbp;
        void *  addr;
        void *  newaddr;
@@ -406,7 +408,7 @@
                        return -1;
                }
                cgd_cipher(cs, newaddr, addr, bp->b_bcount, bn,
-                   DEV_BSIZE, CGD_CIPHER_ENCRYPT);
+                   pdg->pdg_secsize, CGD_CIPHER_ENCRYPT);
        }
 
        nbp->b_data = newaddr;
@@ -437,6 +439,7 @@
        struct  buf *obp = nbp->b_private;
        struct  cgd_softc *cs = getcgd_softc(obp->b_dev);
        struct  dk_softc *dksc = &cs->sc_dksc;
+       struct  dk_geom *pdg = &dksc->sc_geom;
        int s;
 
        KDASSERT(cs);
@@ -461,7 +464,7 @@
 
        if (nbp->b_flags & B_READ)
                cgd_cipher(cs, obp->b_data, obp->b_data, obp->b_bcount,
-                   nbp->b_blkno, DEV_BSIZE, CGD_CIPHER_DECRYPT);
+                   nbp->b_blkno, pdg->pdg_secsize, CGD_CIPHER_DECRYPT);
 
        /* If we allocated memory, free it now... */
        if (nbp->b_data != obp->b_data)
@@ -779,9 +782,9 @@
         *     geometry that we discover from the device.
         */
        pdg = &cs->sc_dksc.sc_geom;
-       pdg->pdg_secsize = DEV_BSIZE;
+       pdg->pdg_secsize = secsize;
        pdg->pdg_ntracks = 1;
-       pdg->pdg_nsectors = 1024 * (1024 / pdg->pdg_secsize);
+       pdg->pdg_nsectors = (1024 * 1024) / pdg->pdg_secsize;
        pdg->pdg_ncylinders = cs->sc_dksc.sc_size / pdg->pdg_nsectors;
 
 bail:
Index: dksubr.c
===================================================================
RCS file: /cvsroot/src/sys/dev/dksubr.c,v
retrieving revision 1.45
diff -u -r1.45 dksubr.c
--- dksubr.c    29 May 2012 10:20:33 -0000      1.45
+++ dksubr.c    2 Jun 2012 14:26:13 -0000
@@ -259,6 +259,7 @@
        int     is_open;
        int     part;
        int     size;
+       struct dk_geom *pdg = &dksc->sc_geom;
 
        if ((dksc->sc_flags & DKF_INITED) == 0)
                return -1;
@@ -274,7 +275,7 @@
                size = -1;
        else
                size = lp->d_partitions[part].p_size *
-                   (lp->d_secsize / DEV_BSIZE);
+                   (lp->d_secsize / pdg->pdg_secsize);
 
        if (!is_open && di->di_close(dev, 0, S_IFBLK, curlwp))
                return 1;