vnds considerd harmful.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

vnds considerd harmful.

Thordur Bjornsson-2
Hi,

Now that I've disallowed swapping to vnd's the purpose
of vnd (vs svnd) is suspect, it serves no purpose other
then providing a different way of doing what svnd does
(which imo, isn't even better).

So, nuke vnds (keep svnds though!).

This will make svndXn the same as vndXn etc. The idea is
that in a few releases we'll simply remove the svnd0 notes.

comments/ok ?


Index: dev/vnd.c
===================================================================
RCS file: /home/thib/cvs/src/sys/dev/vnd.c,v
retrieving revision 1.108
diff -u -p -r1.108 vnd.c
--- dev/vnd.c 2 Apr 2011 15:24:03 -0000 1.108
+++ dev/vnd.c 3 Apr 2011 18:29:52 -0000
@@ -33,25 +33,11 @@
  * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
- *
- * from: Utah $Hdr: vn.c 1.13 94/04/02$
- *
- * @(#)vn.c 8.6 (Berkeley) 4/1/94
  */
 
 /*
- * Vnode disk driver.
- *
- * Block/character interface to a vnode.  Allows one to treat a file
- * as a disk (e.g. build a filesystem in it, mount it, etc.).
- *
- * NOTE 1: This uses either the VOP_BMAP/VOP_STRATEGY interface to the
- * vnode or simple VOP_READ/VOP_WRITE.  The former is suitable for swapping
- * as it doesn't distort the local buffer cache.  The latter is good for
- * building disk images as it keeps the cache consistent after the block
- * device is closed.
+ * There is a security issue involved with this driver.
  *
- * NOTE 2: There is a security issue involved with this driver.
  * Once mounted all access to the contents of the "mapped" file via
  * the special file is controlled by the permissions on the special
  * file, the protection of the mapped file is ignored (effectively,
@@ -102,12 +88,8 @@ int vnddebug = 0x00;
  * DISKUNIT(), but with the minor masked off.
  */
 #define vndunit(x) DISKUNIT(makedev(major(x), minor(x) & 0x7ff))
-#define vndsimple(x) (minor(x) & 0x800)
-
-/* same as MAKEDISKDEV, preserving the vndsimple() property */
 #define VNDLABELDEV(dev) \
- makedev(major(dev), DISKMINOR(vndunit(dev), RAW_PART) | \
-    (vndsimple(dev) ? 0x800 : 0))
+ makedev(major(dev), DISKMINOR(vndunit(dev), RAW_PART))
 
 struct vndbuf {
  struct buf vb_buf;
@@ -145,7 +127,6 @@ struct vnd_softc {
 #define VNF_LABELLING 0x0100
 #define VNF_WLABEL 0x0200
 #define VNF_HAVELABEL 0x0400
-#define VNF_SIMPLE 0x1000
 #define VNF_READONLY 0x2000
 
 #define VNDRW(v) ((v)->sc_flags & VNF_READONLY ? FREAD : FREAD|FWRITE)
@@ -157,7 +138,6 @@ int numvnd = 0;
 void vndattach(int);
 
 void vndclear(struct vnd_softc *);
-void vndstart(struct vnd_softc *, struct buf *);
 int vndsetcred(struct vnd_softc *, struct ucred *);
 void vndiodone(struct buf *);
 void vndshutdown(void);
@@ -232,12 +212,6 @@ vndopen(dev_t dev, int flags, int mode,
  if ((error = vndlock(sc)) != 0)
  return (error);
 
- if (!vndsimple(dev) && sc->sc_vp != NULL &&
-    (sc->sc_vp->v_type != VREG || sc->sc_keyctx != NULL)) {
- error = EINVAL;
- goto bad;
- }
-
  if ((flags & FWRITE) && (sc->sc_flags & VNF_READONLY)) {
  error = EROFS;
  goto bad;
@@ -252,20 +226,11 @@ vndopen(dev_t dev, int flags, int mode,
  part = DISKPART(dev);
  pmask = 1 << part;
 
- /*
- * If any partition is open, all succeeding openings must be of the
- * same type or read-only.
- */
- if (sc->sc_dk.dk_openmask) {
- if (((sc->sc_flags & VNF_SIMPLE) != 0) !=
-    (vndsimple(dev) != 0) && (flags & FWRITE)) {
- error = EBUSY;
- goto bad;
- }
- } else if (vndsimple(dev))
- sc->sc_flags |= VNF_SIMPLE;
- else
- sc->sc_flags &= ~VNF_SIMPLE;
+ /* XXX: OK ?*/
+ if (sc->sc_dk.dk_openmask && (flags & FWRITE)) {
+ error = EBUSY;
+ goto bad;
+ }
 
  /* Check that the partition exists. */
  if (part != RAW_PART &&
@@ -360,30 +325,13 @@ vndclose(dev_t dev, int flags, int mode,
  return (0);
 }
 
-/*
- * Two methods are used, the traditional buffercache bypassing and the
- * newer, cache-coherent on unmount, one.
- *
- * Former method:
- * Break the request into bsize pieces and submit using VOP_BMAP/VOP_STRATEGY.
- * Note that this driver can only be used for swapping over NFS on the hp
- * since nfs_strategy on the vax cannot handle u-areas and page tables.
- *
- * Latter method:
- * Repack the buffer into an uio structure and use VOP_READ/VOP_WRITE to
- * access the underlying file.
- */
 void
 vndstrategy(struct buf *bp)
 {
  int unit = vndunit(bp->b_dev);
  struct vnd_softc *vnd = &vnd_softc[unit];
- struct vndbuf *nbp;
- int bsize;
  off_t bn;
- caddr_t addr;
- size_t resid;
- int sz, flags, error, s;
+ int sz, s;
  struct iovec aiov;
  struct uio auio;
  struct proc *p = curproc;
@@ -441,167 +389,50 @@ vndstrategy(struct buf *bp)
  else
  sz = howmany(bp->b_bcount, DEV_BSIZE);
 
- /* No bypassing of buffer cache?  */
- if (vndsimple(bp->b_dev)) {
- int part = DISKPART(bp->b_dev);
- daddr64_t off = DL_SECTOBLK(vnd->sc_dk.dk_label,
-    DL_GETPOFFSET(&vnd->sc_dk.dk_label->d_partitions[part]));
- aiov.iov_base = bp->b_data;
- auio.uio_resid = aiov.iov_len = bp->b_bcount;
- auio.uio_iov = &aiov;
- auio.uio_iovcnt = 1;
- auio.uio_offset = dbtob((off_t)(bp->b_blkno + off));
- auio.uio_segflg = UIO_SYSSPACE;
- auio.uio_procp = p;
-
- vn_lock(vnd->sc_vp, LK_EXCLUSIVE | LK_RETRY, p);
- if (bp->b_flags & B_READ) {
- auio.uio_rw = UIO_READ;
- bp->b_error = VOP_READ(vnd->sc_vp, &auio, 0,
-    vnd->sc_cred);
- if (vnd->sc_keyctx)
- vndencrypt(vnd, bp->b_data,
-   bp->b_bcount, bp->b_blkno, 0);
- } else {
- if (vnd->sc_keyctx)
- vndencrypt(vnd, bp->b_data,
-   bp->b_bcount, bp->b_blkno, 1);
- auio.uio_rw = UIO_WRITE;
- /*
- * Upper layer has already checked I/O for
- * limits, so there is no need to do it again.
- */
- bp->b_error = VOP_WRITE(vnd->sc_vp, &auio,
-    IO_NOLIMIT, vnd->sc_cred);
- /* Data in buffer cache needs to be in clear */
- if (vnd->sc_keyctx)
- vndencrypt(vnd, bp->b_data,
-   bp->b_bcount, bp->b_blkno, 0);
- }
- VOP_UNLOCK(vnd->sc_vp, 0, p);
- if (bp->b_error)
- bp->b_flags |= B_ERROR;
- bp->b_resid = auio.uio_resid;
- s = splbio();
- biodone(bp);
- splx(s);
-
- return;
- }
-
- if (vnd->sc_vp->v_type != VREG || vnd->sc_keyctx != NULL) {
- bp->b_error = EINVAL;
- bp->b_flags |= B_ERROR;
- s = splbio();
- biodone(bp);
- splx(s);
- return;
- }
-
- /* The old-style buffercache bypassing method.  */
- bn += DL_SECTOBLK(vnd->sc_dk.dk_label,
-    DL_GETPOFFSET(&vnd->sc_dk.dk_label->d_partitions[DISKPART(bp->b_dev)]));
- bn = dbtob(bn);
- bsize = vnd->sc_vp->v_mount->mnt_stat.f_iosize;
- addr = bp->b_data;
- flags = bp->b_flags | B_CALL;
- for (resid = bp->b_resid; resid; resid -= sz) {
- struct vnode *vp;
- daddr64_t nbn;
- int off, nra;
-
- nra = 0;
- vn_lock(vnd->sc_vp, LK_RETRY | LK_EXCLUSIVE, p);
- error = VOP_BMAP(vnd->sc_vp, bn / bsize, &vp, &nbn, &nra);
- VOP_UNLOCK(vnd->sc_vp, 0, p);
- if (error == 0 && (long)nbn == -1)
- error = EIO;
-#ifdef VNDDEBUG
- if (!dovndcluster)
- nra = 0;
-#endif
-
- if ((off = bn % bsize) != 0)
- sz = bsize - off;
- else
- sz = (1 + nra) * bsize;
- if (resid < sz)
- sz = resid;
-
- DNPRINTF(VDB_IO, "vndstrategy: vp %p/%p bn %x/%lld sz %x\n",
-    vnd->sc_vp, vp, bn, nbn, sz);
-
- s = splbio();
- nbp = getvndbuf();
- splx(s);
- nbp->vb_buf.b_flags = flags;
- nbp->vb_buf.b_bcount = sz;
- nbp->vb_buf.b_bufsize = bp->b_bufsize;
- nbp->vb_buf.b_error = 0;
- if (vp->v_type == VBLK || vp->v_type == VCHR)
- nbp->vb_buf.b_dev = vp->v_rdev;
- else
- nbp->vb_buf.b_dev = NODEV;
- nbp->vb_buf.b_data = addr;
- nbp->vb_buf.b_blkno = nbn + btodb(off);
- nbp->vb_buf.b_proc = bp->b_proc;
- nbp->vb_buf.b_iodone = vndiodone;
- nbp->vb_buf.b_vp = vp;
- nbp->vb_buf.b_dirtyoff = bp->b_dirtyoff;
- nbp->vb_buf.b_dirtyend = bp->b_dirtyend;
- nbp->vb_buf.b_validoff = bp->b_validoff;
- nbp->vb_buf.b_validend = bp->b_validend;
- LIST_INIT(&nbp->vb_buf.b_dep);
- nbp->vb_buf.b_bq = NULL;
-
- /* save a reference to the old buffer */
- nbp->vb_obp = bp;
-
+ int part = DISKPART(bp->b_dev);
+ daddr64_t off = DL_SECTOBLK(vnd->sc_dk.dk_label,
+    DL_GETPOFFSET(&vnd->sc_dk.dk_label->d_partitions[part]));
+ aiov.iov_base = bp->b_data;
+ auio.uio_resid = aiov.iov_len = bp->b_bcount;
+ auio.uio_iov = &aiov;
+ auio.uio_iovcnt = 1;
+ auio.uio_offset = dbtob((off_t)(bp->b_blkno + off));
+ auio.uio_segflg = UIO_SYSSPACE;
+ auio.uio_procp = p;
+
+ vn_lock(vnd->sc_vp, LK_EXCLUSIVE | LK_RETRY, p);
+ if (bp->b_flags & B_READ) {
+ auio.uio_rw = UIO_READ;
+ bp->b_error = VOP_READ(vnd->sc_vp, &auio, 0,
+    vnd->sc_cred);
+ if (vnd->sc_keyctx)
+ vndencrypt(vnd, bp->b_data,
+   bp->b_bcount, bp->b_blkno, 0);
+ } else {
+ if (vnd->sc_keyctx)
+ vndencrypt(vnd, bp->b_data,
+   bp->b_bcount, bp->b_blkno, 1);
+ auio.uio_rw = UIO_WRITE;
  /*
- * If there was an error or a hole in the file...punt.
- * Note that we deal with this after the nbp allocation.
- * This ensures that we properly clean up any operations
- * that we have already fired off.
- *
- * XXX we could deal with holes here but it would be
- * a hassle (in the write case).
- * We must still however charge for the write even if there
- * was an error.
+ * Upper layer has already checked I/O for
+ * limits, so there is no need to do it again.
  */
- if (error) {
- nbp->vb_buf.b_error = error;
- nbp->vb_buf.b_flags |= B_ERROR;
- bp->b_resid -= (resid - sz);
- s = splbio();
- /* charge for the write */
- if ((nbp->vb_buf.b_flags & B_READ) == 0)
- nbp->vb_buf.b_vp->v_numoutput++;
- biodone(&nbp->vb_buf);
- splx(s);
- return;
- }
-
- vndstart(vnd, &nbp->vb_buf);
- bn += sz;
- addr += sz;
+ bp->b_error = VOP_WRITE(vnd->sc_vp, &auio,
+    IO_NOLIMIT, vnd->sc_cred);
+ /* Data in buffer cache needs to be in clear */
+ if (vnd->sc_keyctx)
+ vndencrypt(vnd, bp->b_data,
+   bp->b_bcount, bp->b_blkno, 0);
  }
+ VOP_UNLOCK(vnd->sc_vp, 0, p);
+ if (bp->b_error)
+ bp->b_flags |= B_ERROR;
+ bp->b_resid = auio.uio_resid;
+ s = splbio();
+ biodone(bp);
+ splx(s);
 }
 
-void
-vndstart(struct vnd_softc *vnd, struct buf *bp)
-{
- DNPRINTF(VDB_IO,
-    "vndstart(%d): bp %p vp %p blkno %lld addr %p cnt %lx\n",
-    vnd-vnd_softc, bp, bp->b_vp, bp->b_blkno, bp->b_data,
-    bp->b_bcount);
-
- /* Instrumentation. */
- disk_busy(&vnd->sc_dk);
-
- if ((bp->b_flags & B_READ) == 0)
- bp->b_vp->v_numoutput++;
- VOP_STRATEGY(bp);
-}
 
 void
 vndiodone(struct buf *bp)
@@ -726,8 +557,6 @@ vndioctl(dev_t dev, u_long cmd, caddr_t
  case VNDIOCSET:
  if (vnd->sc_flags & VNF_INITED)
  return (EBUSY);
- if (!(vnd->sc_flags & VNF_SIMPLE) && vio->vnd_keylen)
- return (EINVAL);
 
  if ((error = vndlock(vnd)) != 0)
  return (error);
@@ -750,8 +579,7 @@ vndioctl(dev_t dev, u_long cmd, caddr_t
  /* Set disk name depending on how we were created. */
  bzero(vnd->sc_dk_name, sizeof(vnd->sc_dk_name));
  if (snprintf(vnd->sc_dk_name, sizeof(vnd->sc_dk_name),
-    "%svnd%d", ((vnd->sc_flags & VNF_SIMPLE) ? "s" : ""),
-    unit) >= sizeof(vnd->sc_dk_name)) {
+    "svnd%d", unit) >= sizeof(vnd->sc_dk_name)) {
  printf("VNDIOCSET: disk name too long\n");
  vndunlock(vnd);
  return(ENXIO);
@@ -777,13 +605,6 @@ vndioctl(dev_t dev, u_long cmd, caddr_t
  if (error) {
  vndunlock(vnd);
  return (error);
- }
-
- if (nd.ni_vp->v_type != VREG && !vndsimple(dev)) {
- VOP_UNLOCK(nd.ni_vp, 0, p);
- vn_close(nd.ni_vp, VNDRW(vnd), p->p_ucred, p);
- vndunlock(vnd);
- return (EINVAL);
  }
 
  if (nd.ni_vp->v_type == VBLK)

Reply | Threaded
Open this post in threaded view
|

Re: vnds considerd harmful.

Jonathan Thornburg-3
In <http://marc.info/?l=openbsd-tech&m=130200205608892&w=1>,
Thordur Bjornsson <thib () openbsd ! org> wrote:

> Now that I've disallowed swapping to vnd's the purpose    
> of vnd (vs svnd) is suspect, it serves no purpose other
> then providing a different way of doing what svnd does
> (which imo, isn't even better).
>
> So, nuke vnds (keep svnds though!).
>
> This will make svndXn the same as vndXn etc. The idea is
> that in a few releases we'll simply remove the svnd0 notes.
>
> comments/ok ?

Could you clarify the semantics of the "in a few releases" plan?
That is, are you proposing that the "in a few releases" OpenBSD will have
(a) vnd == today's svnd,
(b) vnd == today's vnd,
(c) vnd == some sort of merging of today's vnd and today's svnd, or
(d) something else which hasn't occured to me yet

I sort of think you're proposing (a), but I'm not entirely sure that I'm
parsing your wording correctly...  [Hmm, I wonder if my failure-to-parse
is related to a recent bout of perl hashes holding references to anonymous
hashes holding references to anonymous lists. :) ]

thanks, ciao,

--
-- "Jonathan Thornburg [remove -animal to reply]" <[hidden email]>
   Dept of Astronomy & IUCSS, Indiana University, Bloomington, Indiana, USA
   "Washing one's hands of the conflict between the powerful and the
    powerless means to side with the powerful, not to be neutral."
                                      -- quote by Freire / poster by Oxfam

Reply | Threaded
Open this post in threaded view
|

Re: vnds considerd harmful.

Thordur Bjornsson-2
On Wed, Apr 06, 2011 at 04:25:15PM -0400, Jonathan Thornburg wrote:

> In <http://marc.info/?l=openbsd-tech&m=130200205608892&w=1>,
> Thordur Bjornsson <thib () openbsd ! org> wrote:
> > Now that I've disallowed swapping to vnd's the purpose    
> > of vnd (vs svnd) is suspect, it serves no purpose other
> > then providing a different way of doing what svnd does
> > (which imo, isn't even better).
> >
> > So, nuke vnds (keep svnds though!).
> >
> > This will make svndXn the same as vndXn etc. The idea is
> > that in a few releases we'll simply remove the svnd0 notes.
                                                         ^^^^^ <- nodes.

With this diff svnd0 == vnd0 in your /dev.
 
> Could you clarify the semantics of the "in a few releases" plan?
> That is, are you proposing that the "in a few releases" OpenBSD will have
> (a) vnd == today's svnd,
    bingo.

> (b) vnd == today's vnd,
> (c) vnd == some sort of merging of today's vnd and today's svnd, or
> (d) something else which hasn't occured to me yet
 
> I sort of think you're proposing (a), but I'm not entirely sure that I'm
> parsing your wording correctly...  [Hmm, I wonder if my failure-to-parse
> is related to a recent bout of perl hashes holding references to anonymous
> hashes holding references to anonymous lists. :) ]

So, yeah. vnd's will become today's svnd0's and the old style
bypassing of the buffer cache is gone (leaving only svnd0s).

Then in a few releases, the svnd device nodes will be removed.