Date: Tue, 17 Apr 2001 10:58:24 +0100 From: Brian Somers <brian@Awfulhak.org> To: Dima Dorfman <dima@unixfreak.org> Cc: Brian Somers <brian@Awfulhak.org>, hackers@FreeBSD.ORG, brian@Awfulhak.org Subject: Re: Patch to make snp(4) devfs-friendly Message-ID: <200104170958.f3H9wOr62584@hak.lan.Awfulhak.org> In-Reply-To: Message from Dima Dorfman <dima@unixfreak.org> of "Tue, 17 Apr 2001 02:22:23 PDT." <20010417092223.9AC973E09@bazooka.unixfreak.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This looks good. I'd say you should commit the change yourself -
feel free to say it's reviewed by me.
As an aside, when I did this to if_tun, I chose to do all the
destroy_dev()s at module unload time (I guess the snp device could do
with a MODULE_DECLARE). This allows the administrator to relax the
driver-chosen permissions on tun devices with something like
kldload if_tun
>/dev/tun0
chmod blah /dev/tun0
The destroy_dev() added to snpclose() prevents this.
Having said all that, it's unlikely that anyone would want to relax
the permissions a snoop device that wasn't already opened, so the
destroy_dev() looks ok here in practice.
> Brian Somers <brian@Awfulhak.org> writes:
> > I haven't actually tested the code, but looking at the patch, I think
> > there's a problem with it...
> >
> > Specifically, on a non-devfs system - where the device nodes are
> > created with mknod(1), snp_clone() isn't going to be called before
> > snpopen().
> >
> > I've (ab)used drv2 as a flag to say whether make_dev() has been
> > called in net/if_tun.c, but I'm not sure if this is the right answer
> > either :*]
>
> I think the correct approach is to check for !(si_flags & SI_NAMED) as
> done in rev. 1.68 of sys/net/bpf.c (that rev. has a precedence bug,
> which has since been fixed). That was written by phk, who I take to
> be pretty authoritative on the subject; looking at the places SI_NAMED
> is used also supports this conclusion.
>
> A corrected patch is attached (although I didn't test it in the
> non-devfs case).
>
> Thanks!
>
> Dima Dorfman
> dima@unixfreak.org
>
>
> Index: tty_snoop.c
> ===================================================================
> RCS file: /st/src/FreeBSD/src/sys/kern/tty_snoop.c,v
> retrieving revision 1.52
> diff -u -r1.52 tty_snoop.c
> --- tty_snoop.c 2001/03/26 12:41:01 1.52
> +++ tty_snoop.c 2001/04/17 09:16:52
> @@ -287,10 +287,10 @@
> return (error);
>
> if (dev->si_drv1 == NULL) {
> - int mynor = minor(dev);
> -
> + if (!(dev->si_flags & SI_NAMED))
> + make_dev(&snp_cdevsw, minor(dev), UID_ROOT, GID_WHEEL,
> + 0600, "snp%d", dev2unit(dev));
> dev->si_drv1 = snp = malloc(sizeof(*snp), M_SNP, M_WAITOK|M_ZERO);
> - make_dev(&snp_cdevsw, mynor, 0, 0, 0600, "snp%d", mynor);
> } else
> return (EBUSY);
>
> @@ -365,6 +365,7 @@
> free(snp->snp_buf, M_SNP);
> snp->snp_flags &= ~SNOOP_OPEN;
> dev->si_drv1 = NULL;
> + destroy_dev(dev);
>
> return (snp_detach(snp));
> }
> @@ -505,10 +506,25 @@
> static void snp_drvinit __P((void *unused));
>
> static void
> +snp_clone(void *arg, char *name, int namelen, dev_t *dev)
> +{
> + int u;
> +
> + if (*dev != NODEV)
> + return;
> + if (dev_stdclone(name, NULL, "snp", &u) != 1)
> + return;
> + *dev = make_dev(&snp_cdevsw, unit2minor(u), UID_ROOT, GID_WHEEL, 0600,
> + "snp%d", u);
> + return;
> +}
> +
> +static void
> snp_drvinit(unused)
> void *unused;
> {
>
> + EVENTHANDLER_REGISTER(dev_clone, snp_clone, 0, 1000);
> cdevsw_add(&snp_cdevsw);
> }
>
--
Brian <brian@Awfulhak.org> <brian@[uk.]FreeBSD.org>
<http://www.Awfulhak.org>; <brian@[uk.]OpenBSD.org>
Don't _EVER_ lose your sense of humour !
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200104170958.f3H9wOr62584>