From owner-svn-src-all@freebsd.org Wed Aug 19 18:23:41 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 13C6F3C6A2E for ; Wed, 19 Aug 2020 18:23:41 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound2k.ore.mailhop.org (outbound2k.ore.mailhop.org [54.148.219.64]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4BWx4B5p4gz4PD5 for ; Wed, 19 Aug 2020 18:23:38 +0000 (UTC) (envelope-from ian@freebsd.org) ARC-Seal: i=1; a=rsa-sha256; t=1597861417; cv=none; d=outbound.mailhop.org; s=arc-outbound20181012; b=ZUMZGktlGNh7ZboGMkmUAQWfOSH1ozXjJATrx8kU6BHLghZ4gfi/i2caRzYfpOAy/RmBU6fk+jQhD cdk2X7OPWs4WO4fg4W4jE8DW+SIGWh3GGXvx2/wr8QBLPuzCJLJzHhPIIXLRf+EqT+wGlfOj1Wc9H7 hG1rX6wxpvnYM0YWMQbh716mYomUxqezfDxY4L5l9TNM2P76+V/5ibTprVZoCc8rNvEmK/gjZLqkNA lZGm9sL2J02kp4eP4u2KK1nt996XlIYg5YVf9+iHTdizn8K8FJTdx+dk9xhSZy7BtDw836qQEExB96 wfAihjSWYfXARIoWgtoF+/iFEiat08Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=arc-outbound20181012; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:cc:to:from:subject:message-id:dkim-signature:from; bh=SUP6PH6Y2mdMmhex5Eam2wv1wD4p095s43LGqJpqms4=; b=PqO19waxag9D8QBWc6Djt5zGn+l6YqsE+gVKAdcBk/uG/vDsiffajV50piBLPmr4u97MhfKr9ic5L wur4JJgQ4F7+6wltGRUIJLbucVovzYJ6pQZJgyuc9QwdJyhwGPo7r3ULjmi+t3OLjaduC4SlwbWK3o 63Uf6BWge24/ylSSLRGmQw1+xWjJM15yHwpqpipgYCIr8NS2DA7hiQxl6yiQOXWhomkncMGTqH0q7M LDcay6Q2BNy5KH0OR9mSovNRm44pm90tYBYmG7S2r3jr/TDOwGh/tjXTloBRfJ/dXEllLLlfGcmZCb MjFOEB71kcDkWQiJilGxCR6rXMj5m3w== ARC-Authentication-Results: i=1; outbound4.ore.mailhop.org; spf=softfail smtp.mailfrom=freebsd.org smtp.remote-ip=67.177.211.60; dmarc=none header.from=freebsd.org; arc=none header.oldest-pass=0; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=dkim-high; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:cc:to:from:subject:message-id:from; bh=SUP6PH6Y2mdMmhex5Eam2wv1wD4p095s43LGqJpqms4=; b=uJ5wct9l8yxU/xAIGR0TEYy6N1z4WDBw0QLLZC7tKm7/vaAQh7dSu5tgSD6D4NtB97IwSVyIRN9s/ NAvbXpfeuJA1BlREa1cKyUxHPtMSdDOvE9Q06tOJdDn5L1jD3Z/jl4m1RLFmzuFuOBh/11hSPE6QSm PJTBrPQ2tpm64gKHcVSKZzq/3s5Q9uRLyJwc8nWs44nCQZCsDwKyHWz8dpb5Va4T+oL1rhS0XR/NFJ v1pBh6uUbG7FHSng6dIG94PGjbvj4pp9fmIcpOOIa7izuYfBnzgf7jv8WQqwuFa3Ny7rtpTSaHChg1 IFnWcDmHtDaodLuPKuXYhr5X2kz2ZOw== X-MHO-RoutePath: aGlwcGll X-MHO-User: 177e0aa3-e249-11ea-b630-6b8aa7872eb8 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (c-67-177-211-60.hsd1.co.comcast.net [67.177.211.60]) by outbound4.ore.mailhop.org (Halon) with ESMTPSA id 177e0aa3-e249-11ea-b630-6b8aa7872eb8; Wed, 19 Aug 2020 18:23:35 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id 07JI0Ama089633; Wed, 19 Aug 2020 12:00:32 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: Subject: Re: svn commit: r364402 - head/sys/kern From: Ian Lepore To: Shawn Webb , Warner Losh Cc: Warner Losh , src-committers , svn-src-all , svn-src-head Date: Wed, 19 Aug 2020 12:00:10 -0600 In-Reply-To: <20200819175447.pjoa2pmusdbql22q@mutt-hbsd> References: <202008191710.07JHA5Rk008764@repo.freebsd.org> <20200819172613.vdyutsn6a4w5fbqr@mutt-hbsd> <20200819174808.ts3i72vzqsx3sq5o@mutt-hbsd> <20200819175447.pjoa2pmusdbql22q@mutt-hbsd> Content-Type: text/plain; charset="ASCII" X-Mailer: Evolution 3.28.5 FreeBSD GNOME Team Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4BWx4B5p4gz4PD5 X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [0.00 / 15.00]; ASN(0.00)[asn:16509, ipnet:54.148.0.0/15, country:US]; local_wl_from(0.00)[freebsd.org] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Aug 2020 18:23:41 -0000 On Wed, 2020-08-19 at 13:54 -0400, Shawn Webb wrote: > On Wed, Aug 19, 2020 at 11:51:10AM -0600, Warner Losh wrote: > > On Wed, Aug 19, 2020 at 11:48 AM Shawn Webb < > > shawn.webb@hardenedbsd.org> > > wrote: > > > > > On Wed, Aug 19, 2020 at 11:44:42AM -0600, Warner Losh wrote: > > > > On Wed, Aug 19, 2020 at 11:26 AM Shawn Webb < > > > > shawn.webb@hardenedbsd.org> > > > > wrote: > > > > > > > > > On Wed, Aug 19, 2020 at 05:10:05PM +0000, Warner Losh wrote: > > > > > > Author: imp > > > > > > Date: Wed Aug 19 17:10:04 2020 > > > > > > New Revision: 364402 > > > > > > URL: https://svnweb.freebsd.org/changeset/base/364402 > > > > > > > > > > > > Log: > > > > > > Add VFS FS events for mount and unmount to devctl/devd > > > > > > > > > > > > Report when a filesystem is mounted, remounted or > > > > > > unmounted via > > > > > > devd, > > > > > along with > > > > > > details about the mount point and mount options. > > > > > > > > > > > > Discussed with: kib@ > > > > > > Reviewed by: kirk@ (prior version) > > > > > > Sponsored by: Netflix > > > > > > Diffential Revision: https://reviews.freebsd.org/D25969 > > > > > > > > > > > > Modified: > > > > > > head/sys/kern/vfs_mount.c > > > > > > > > > > > > Modified: head/sys/kern/vfs_mount.c > > > > > > > > > > > > ================================================================= > > > ============= > > > > > > --- head/sys/kern/vfs_mount.c Wed Aug 19 17:09:58 2020 > > > > > > (r364401) > > > > > > +++ head/sys/kern/vfs_mount.c Wed Aug 19 17:10:04 2020 > > > > > > (r364402) > > > > > > @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$"); > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > +#include > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > @@ -101,6 +102,8 @@ MTX_SYSINIT(mountlist, &mountlist_mtx, > > > > > > "mountlist", > > > > > MT > > > > > > EVENTHANDLER_LIST_DEFINE(vfs_mounted); > > > > > > EVENTHANDLER_LIST_DEFINE(vfs_unmounted); > > > > > > > > > > > > +static void dev_vfs_event(const char *type, struct mount > > > > > > *mp, bool > > > > > > > > > > donew); > > > > > > + > > > > > > /* > > > > > > * Global opts, taken by all filesystems > > > > > > */ > > > > > > @@ -1020,6 +1023,7 @@ vfs_domount_first( > > > > > > VOP_UNLOCK(vp); > > > > > > EVENTHANDLER_DIRECT_INVOKE(vfs_mounted, mp, newdp, > > > > > > td); > > > > > > VOP_UNLOCK(newdp); > > > > > > + dev_vfs_event("MOUNT", mp, false); > > > > > > mountcheckdirs(vp, newdp); > > > > > > vn_seqc_write_end(vp); > > > > > > vn_seqc_write_end(newdp); > > > > > > @@ -1221,6 +1225,7 @@ vfs_domount_update( > > > > > > if (error != 0) > > > > > > goto end; > > > > > > > > > > > > + dev_vfs_event("REMOUNT", mp, true); > > > > > > if (mp->mnt_opt != NULL) > > > > > > vfs_freeopts(mp->mnt_opt); > > > > > > mp->mnt_opt = mp->mnt_optnew; > > > > > > @@ -1839,6 +1844,7 @@ dounmount(struct mount *mp, int > > > > > > flags, struct > > > > > > > > > > thread * > > > > > > TAILQ_REMOVE(&mountlist, mp, mnt_list); > > > > > > mtx_unlock(&mountlist_mtx); > > > > > > EVENTHANDLER_DIRECT_INVOKE(vfs_unmounted, mp, td); > > > > > > + dev_vfs_event("UNMOUNT", mp, false); > > > > > > if (coveredvp != NULL) { > > > > > > coveredvp->v_mountedhere = NULL; > > > > > > vn_seqc_write_end(coveredvp); > > > > > > @@ -2425,4 +2431,72 @@ kernel_vmount(int flags, ...) > > > > > > > > > > > > error = kernel_mount(ma, flags); > > > > > > return (error); > > > > > > +} > > > > > > + > > > > > > +/* Map from mount options to printable formats. */ > > > > > > +static struct mntoptnames optnames[] = { > > > > > > + MNTOPT_NAMES > > > > > > +}; > > > > > > + > > > > > > +static void > > > > > > +dev_vfs_event_mntopt(struct sbuf *sb, const char *what, > > > > > > struct > > > > > > > > > > vfsoptlist *opts) > > > > > > +{ > > > > > > + struct vfsopt *opt; > > > > > > + > > > > > > + if (opts == NULL || TAILQ_EMPTY(opts)) > > > > > > + return; > > > > > > + sbuf_printf(sb, " %s=\"", what); > > > > > > + TAILQ_FOREACH(opt, opts, link) { > > > > > > + if (opt->name[0] == '\0' || (opt->len > 0 && > > > > > > *(char > > > > > > > > > > *)opt->value == '\0')) > > > > > > + continue; > > > > > > + devctl_safe_quote_sb(sb, opt->name); > > > > > > + if (opt->len > 0) { > > > > > > + sbuf_putc(sb, '='); > > > > > > + devctl_safe_quote_sb(sb, opt->value); > > > > > > + } > > > > > > + sbuf_putc(sb, ';'); > > > > > > + } > > > > > > + sbuf_putc(sb, '"'); > > > > > > +} > > > > > > + > > > > > > +#define DEVCTL_LEN 1024 > > > > > > +static void > > > > > > +dev_vfs_event(const char *type, struct mount *mp, bool > > > > > > donew) > > > > > > +{ > > > > > > + const uint8_t *cp; > > > > > > + struct mntoptnames *fp; > > > > > > + struct sbuf sb; > > > > > > + struct statfs *sfp = &mp->mnt_stat; > > > > > > + char *buf; > > > > > > + > > > > > > + buf = malloc(DEVCTL_LEN, M_MOUNT, M_WAITOK); > > > > > > + if (buf == NULL) > > > > > > + return; > > > > > > > > > > buf can't be NULL. > > > > > > > > > > > > > The bug here is that M_NOWAIT should have been specified in the > > > > malloc. > > > > > > > > > > > > > > + sbuf_new(&sb, buf, DEVCTL_LEN, SBUF_FIXEDLEN); > > > > > > + sbuf_cpy(&sb, "mount-point=\""); > > > > > > + devctl_safe_quote_sb(&sb, sfp->f_mntonname); > > > > > > + sbuf_cat(&sb, "\" mount-dev=\""); > > > > > > + devctl_safe_quote_sb(&sb, sfp->f_mntfromname); > > > > > > + sbuf_cat(&sb, "\" mount-type=\""); > > > > > > + devctl_safe_quote_sb(&sb, sfp->f_fstypename); > > > > > > + sbuf_cat(&sb, "\" fsid=0x"); > > > > > > + cp = (const uint8_t *)&sfp->f_fsid.val[0]; > > > > > > + for (int i = 0; i < sizeof(sfp->f_fsid); i++) > > > > > > + sbuf_printf(&sb, "%02x", cp[i]); > > > > > > + sbuf_printf(&sb, " owner=%u flags=\"", sfp->f_owner); > > > > > > + for (fp = optnames; fp->o_opt != 0; fp++) { > > > > > > + if ((mp->mnt_flag & fp->o_opt) != 0) { > > > > > > + sbuf_cat(&sb, fp->o_name); > > > > > > + sbuf_putc(&sb, ';'); > > > > > > + } > > > > > > + } > > > > > > + sbuf_putc(&sb, '"'); > > > > > > + dev_vfs_event_mntopt(&sb, "opt", mp->mnt_opt); > > > > > > + if (donew) > > > > > > + dev_vfs_event_mntopt(&sb, "optnew", mp- > > > > > > >mnt_optnew); > > > > > > + sbuf_finish(&sb); > > > > > > + > > > > > > + devctl_notify("VFS", "FS", type, sbuf_data(&sb)); > > > > > > + sbuf_delete(&sb); > > > > > > + free(buf, M_MOUNT); > > > > > > } > > > > > > > > > > I don't really see much attention paid to checking for sbuf > > > > > overflow. > > > > > Could that cause issues, especially in case of impartial > > > > > quotation > > > > > termination? Could not performing those checks have security > > > > > implications? Would performing those checks adhere to good > > > > > code > > > > > development practices? > > > > > > > > > > > > > sbuf doesn't overflow. It is safe from that perspective. The > > > > code should > > > > just not send it if there's an overflow... There almost > > > > certainly won't > > > > > > be > > > > one in practice given the buffer size, though. > > > > > > You're right that sbuf can't overflow. However, assuming that it > > > hasn't reached its fixed size specified above and continuing on > > > as if > > > there's still space left could lead to... interesting behavior. > > > > > > > Right, which is why we should check. > > > > See https://reviews.freebsd.org/D26122 for the proper tweak (imho). > > Why spend so much time in the kernel just to fail at the end? Why not > fail fast, checking for sbuf_* returning -1 along the way? > Why complicate the code with micro-optimizations that just clutter things up on a code path that is rarely executed, to check for a condition that will never happen, to save a few nanoseconds if it ever does? Simpler is better. -- Ian