From owner-freebsd-current@freebsd.org Tue Mar 29 00:26:41 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 08C3BAD5191 for ; Tue, 29 Mar 2016 00:26:41 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id E687F11C1 for ; Tue, 29 Mar 2016 00:26:40 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: by mailman.ysv.freebsd.org (Postfix) id E5D16AD518D; Tue, 29 Mar 2016 00:26:40 +0000 (UTC) Delivered-To: current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E5643AD518C for ; Tue, 29 Mar 2016 00:26:40 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id CDBE611C0 for ; Tue, 29 Mar 2016 00:26:40 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id u2T0QdWU049305 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 28 Mar 2016 17:26:39 -0700 (PDT) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id u2T0Qdvm049304; Mon, 28 Mar 2016 17:26:39 -0700 (PDT) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Mon, 28 Mar 2016 17:26:39 -0700 From: Gleb Smirnoff To: Vitalij Satanivskij Cc: current@freebsd.org Subject: Re: CURRENT r296381 panic in vn_sendfile (/usr/src/sys/kern/kern_sendfile.c:833) Message-ID: <20160329002639.GD2616@FreeBSD.org> References: <20160304124053.GA25071@hell.ukr.net> <20160323181131.GN2616@FreeBSD.org> <20160323235925.GT2616@FreeBSD.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="4ndw/alBWmZEhfcZ" Content-Disposition: inline In-Reply-To: <20160323235925.GT2616@FreeBSD.org> User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Mar 2016 00:26:41 -0000 --4ndw/alBWmZEhfcZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Vitalij, here is latest version of the patch. If you already run the previous one, no need to switch to this one, keep running as is. The update covers only FreeBSD 4 and i386 compatibilties. current@, a review is appreciated. The patch not only fixes a recent bug, but also fixes a long standing problem that headers were not checked against socket buffer size. One could push unlimited data into sendfile() with headers. The patch also pushes also compat code under ifdef, so it is cut away if you aren't interested in COMPAT_FREEBSD4. On Wed, Mar 23, 2016 at 04:59:25PM -0700, Gleb Smirnoff wrote: T> Vitalij, T> T> although the first patch should fixup the panic, can you please T> instead run this one. And if it is possible, can you please T> monitor this sysctl: T> T> sysctl kern.ipc.sf_long_headers T> T> T> -- T> Totus tuus, Glebius. T> Index: sys/kern/kern_descrip.c T> =================================================================== T> --- sys/kern/kern_descrip.c (revision 297217) T> +++ sys/kern/kern_descrip.c (working copy) T> @@ -3958,7 +3958,7 @@ badfo_chown(struct file *fp, uid_t uid, gid_t gid, T> static int T> badfo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, T> struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, T> - int kflags, struct thread *td) T> + struct thread *td) T> { T> T> return (EBADF); T> @@ -4044,7 +4044,7 @@ invfo_chown(struct file *fp, uid_t uid, gid_t gid, T> int T> invfo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, T> struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, T> - int kflags, struct thread *td) T> + struct thread *td) T> { T> T> return (EINVAL); T> Index: sys/kern/kern_sendfile.c T> =================================================================== T> --- sys/kern/kern_sendfile.c (revision 297217) T> +++ sys/kern/kern_sendfile.c (working copy) T> @@ -95,6 +95,7 @@ struct sendfile_sync { T> }; T> T> counter_u64_t sfstat[sizeof(struct sfstat) / sizeof(uint64_t)]; T> +static counter_u64_t sf_long_headers; /* QQQGL */ T> T> static void T> sfstat_init(const void *unused) T> @@ -102,6 +103,7 @@ sfstat_init(const void *unused) T> T> COUNTER_ARRAY_ALLOC(sfstat, sizeof(struct sfstat) / sizeof(uint64_t), T> M_WAITOK); T> + sf_long_headers = counter_u64_alloc(M_WAITOK); /* QQQGL */ T> } T> SYSINIT(sfstat, SI_SUB_MBUF, SI_ORDER_FIRST, sfstat_init, NULL); T> T> @@ -117,6 +119,8 @@ sfstat_sysctl(SYSCTL_HANDLER_ARGS) T> } T> SYSCTL_PROC(_kern_ipc, OID_AUTO, sfstat, CTLTYPE_OPAQUE | CTLFLAG_RW, T> NULL, 0, sfstat_sysctl, "I", "sendfile statistics"); T> +SYSCTL_COUNTER_U64(_kern_ipc, OID_AUTO, sf_long_headers, CTLFLAG_RW, T> + &sf_long_headers, "times headers did not fit into socket buffer"); T> T> /* T> * Detach mapped page and release resources back to the system. Called T> @@ -516,7 +520,7 @@ sendfile_getsock(struct thread *td, int s, struct T> int T> vn_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, T> struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, T> - int kflags, struct thread *td) T> + struct thread *td) T> { T> struct file *sock_fp; T> struct vnode *vp; T> @@ -534,7 +538,7 @@ vn_sendfile(struct file *fp, int sockfd, struct ui T> so = NULL; T> m = mh = NULL; T> sfs = NULL; T> - sbytes = 0; T> + hdrlen = sbytes = 0; T> softerr = 0; T> T> error = sendfile_getobj(td, fp, &obj, &vp, &shmfd, &obj_size, &bsize); T> @@ -560,26 +564,6 @@ vn_sendfile(struct file *fp, int sockfd, struct ui T> cv_init(&sfs->cv, "sendfile"); T> } T> T> - /* If headers are specified copy them into mbufs. */ T> - if (hdr_uio != NULL && hdr_uio->uio_resid > 0) { T> - hdr_uio->uio_td = td; T> - hdr_uio->uio_rw = UIO_WRITE; T> - /* T> - * In FBSD < 5.0 the nbytes to send also included T> - * the header. If compat is specified subtract the T> - * header size from nbytes. T> - */ T> - if (kflags & SFK_COMPAT) { T> - if (nbytes > hdr_uio->uio_resid) T> - nbytes -= hdr_uio->uio_resid; T> - else T> - nbytes = 0; T> - } T> - mh = m_uiotombuf(hdr_uio, M_WAITOK, 0, 0, 0); T> - hdrlen = m_length(mh, &mhtail); T> - } else T> - hdrlen = 0; T> - T> rem = nbytes ? omin(nbytes, obj_size - offset) : obj_size - offset; T> T> /* T> @@ -668,11 +652,23 @@ retry_space: T> SOCKBUF_UNLOCK(&so->so_snd); T> T> /* T> - * Reduce space in the socket buffer by the size of T> - * the header mbuf chain. T> - * hdrlen is set to 0 after the first loop. T> + * At the beginning of the first loop check if any headers T> + * are specified and copy them into mbufs. Reduce space in T> + * the socket buffer by the size of the header mbuf chain. T> + * Clear hdr_uio here and hdrlen at the end of the first loop. T> */ T> - space -= hdrlen; T> + if (hdr_uio != NULL) { T> + hdr_uio->uio_td = td; T> + hdr_uio->uio_rw = UIO_WRITE; T> + /* QQQGL remove counter */ T> + if (space < hdr_uio->uio_resid) T> + counter_u64_add(sf_long_headers, 1); T> + hdr_uio->uio_resid = min(hdr_uio->uio_resid, space); T> + mh = m_uiotombuf(hdr_uio, M_WAITOK, 0, 0, 0); T> + hdrlen = m_length(mh, &mhtail); T> + space -= hdrlen; T> + hdr_uio = NULL; T> + } T> T> if (vp != NULL) { T> error = vn_lock(vp, LK_SHARED); T> @@ -944,6 +940,17 @@ sendfile(struct thread *td, struct sendfile_args * T> &hdr_uio); T> if (error != 0) T> goto out; T> + /* T> + * In FBSD < 5.0 the nbytes to send also included T> + * the header. If compat is specified subtract the T> + * header size from nbytes. T> + */ T> + if (compat) { T> + if (uap->nbytes > hdr_uio->uio_resid) T> + uap->nbytes -= hdr_uio->uio_resid; T> + else T> + uap->nbytes = 0; T> + } T> } T> if (hdtr.trailers != NULL) { T> error = copyinuio(hdtr.trailers, hdtr.trl_cnt, T> @@ -965,7 +972,7 @@ sendfile(struct thread *td, struct sendfile_args * T> } T> T> error = fo_sendfile(fp, uap->s, hdr_uio, trl_uio, uap->offset, T> - uap->nbytes, &sbytes, uap->flags, compat ? SFK_COMPAT : 0, td); T> + uap->nbytes, &sbytes, uap->flags, td); T> fdrop(fp, td); T> T> if (uap->sbytes != NULL) T> Index: sys/sys/file.h T> =================================================================== T> --- sys/sys/file.h (revision 297217) T> +++ sys/sys/file.h (working copy) T> @@ -112,7 +112,7 @@ typedef int fo_chown_t(struct file *fp, uid_t uid, T> struct ucred *active_cred, struct thread *td); T> typedef int fo_sendfile_t(struct file *fp, int sockfd, struct uio *hdr_uio, T> struct uio *trl_uio, off_t offset, size_t nbytes, T> - off_t *sent, int flags, int kflags, struct thread *td); T> + off_t *sent, int flags, struct thread *td); T> typedef int fo_seek_t(struct file *fp, off_t offset, int whence, T> struct thread *td); T> typedef int fo_fill_kinfo_t(struct file *fp, struct kinfo_file *kif, T> @@ -376,11 +376,11 @@ fo_chown(struct file *fp, uid_t uid, gid_t gid, st T> static __inline int T> fo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, T> struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, T> - int kflags, struct thread *td) T> + struct thread *td) T> { T> T> return ((*fp->f_ops->fo_sendfile)(fp, sockfd, hdr_uio, trl_uio, offset, T> - nbytes, sent, flags, kflags, td)); T> + nbytes, sent, flags, td)); T> } T> T> static __inline int T> Index: sys/sys/socket.h T> =================================================================== T> --- sys/sys/socket.h (revision 297217) T> +++ sys/sys/socket.h (working copy) T> @@ -594,7 +594,6 @@ struct sf_hdtr { T> #define SF_FLAGS(rh, flags) (((rh) << 16) | (flags)) T> T> #ifdef _KERNEL T> -#define SFK_COMPAT 0x00000001 T> #define SF_READAHEAD(flags) ((flags) >> 16) T> #endif /* _KERNEL */ T> T> _______________________________________________ T> freebsd-current@freebsd.org mailing list T> https://lists.freebsd.org/mailman/listinfo/freebsd-current T> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org" -- Totus tuus, Glebius. --4ndw/alBWmZEhfcZ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="sendfile.diff" Index: sys/compat/freebsd32/freebsd32_misc.c =================================================================== --- sys/compat/freebsd32/freebsd32_misc.c (revision 297366) +++ sys/compat/freebsd32/freebsd32_misc.c (working copy) @@ -1653,6 +1653,19 @@ freebsd32_do_sendfile(struct thread *td, hdtr32.hdr_cnt, &hdr_uio); if (error) goto out; +#ifdef COMPAT_FREEBSD4 + /* + * In FreeBSD < 5.0 the nbytes to send also included + * the header. If compat is specified subtract the + * header size from nbytes. + */ + if (compat) { + if (uap->nbytes > hdr_uio->uio_resid) + uap->nbytes -= hdr_uio->uio_resid; + else + uap->nbytes = 0; + } +#endif } if (hdtr.trailers != NULL) { iov32 = PTRIN(hdtr32.trailers); @@ -1670,7 +1683,7 @@ freebsd32_do_sendfile(struct thread *td, goto out; error = fo_sendfile(fp, uap->s, hdr_uio, trl_uio, offset, - uap->nbytes, &sbytes, uap->flags, compat ? SFK_COMPAT : 0, td); + uap->nbytes, &sbytes, uap->flags, td); fdrop(fp, td); if (uap->sbytes != NULL) Index: sys/kern/kern_descrip.c =================================================================== --- sys/kern/kern_descrip.c (revision 297366) +++ sys/kern/kern_descrip.c (working copy) @@ -3958,7 +3958,7 @@ badfo_chown(struct file *fp, uid_t uid, gid_t gid, static int badfo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, - int kflags, struct thread *td) + struct thread *td) { return (EBADF); @@ -4044,7 +4044,7 @@ invfo_chown(struct file *fp, uid_t uid, gid_t gid, int invfo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, - int kflags, struct thread *td) + struct thread *td) { return (EINVAL); Index: sys/kern/kern_sendfile.c =================================================================== --- sys/kern/kern_sendfile.c (revision 297366) +++ sys/kern/kern_sendfile.c (working copy) @@ -95,6 +95,7 @@ struct sendfile_sync { }; counter_u64_t sfstat[sizeof(struct sfstat) / sizeof(uint64_t)]; +static counter_u64_t sf_long_headers; /* QQQGL */ static void sfstat_init(const void *unused) @@ -102,6 +103,7 @@ sfstat_init(const void *unused) COUNTER_ARRAY_ALLOC(sfstat, sizeof(struct sfstat) / sizeof(uint64_t), M_WAITOK); + sf_long_headers = counter_u64_alloc(M_WAITOK); /* QQQGL */ } SYSINIT(sfstat, SI_SUB_MBUF, SI_ORDER_FIRST, sfstat_init, NULL); @@ -117,6 +119,9 @@ sfstat_sysctl(SYSCTL_HANDLER_ARGS) } SYSCTL_PROC(_kern_ipc, OID_AUTO, sfstat, CTLTYPE_OPAQUE | CTLFLAG_RW, NULL, 0, sfstat_sysctl, "I", "sendfile statistics"); +/* QQQGL */ +SYSCTL_COUNTER_U64(_kern_ipc, OID_AUTO, sf_long_headers, CTLFLAG_RW, + &sf_long_headers, "times headers did not fit into socket buffer"); /* * Detach mapped page and release resources back to the system. Called @@ -516,7 +521,7 @@ sendfile_getsock(struct thread *td, int s, struct int vn_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, - int kflags, struct thread *td) + struct thread *td) { struct file *sock_fp; struct vnode *vp; @@ -534,7 +539,7 @@ vn_sendfile(struct file *fp, int sockfd, struct ui so = NULL; m = mh = NULL; sfs = NULL; - sbytes = 0; + hdrlen = sbytes = 0; softerr = 0; error = sendfile_getobj(td, fp, &obj, &vp, &shmfd, &obj_size, &bsize); @@ -560,26 +565,6 @@ vn_sendfile(struct file *fp, int sockfd, struct ui cv_init(&sfs->cv, "sendfile"); } - /* If headers are specified copy them into mbufs. */ - if (hdr_uio != NULL && hdr_uio->uio_resid > 0) { - hdr_uio->uio_td = td; - hdr_uio->uio_rw = UIO_WRITE; - /* - * In FBSD < 5.0 the nbytes to send also included - * the header. If compat is specified subtract the - * header size from nbytes. - */ - if (kflags & SFK_COMPAT) { - if (nbytes > hdr_uio->uio_resid) - nbytes -= hdr_uio->uio_resid; - else - nbytes = 0; - } - mh = m_uiotombuf(hdr_uio, M_WAITOK, 0, 0, 0); - hdrlen = m_length(mh, &mhtail); - } else - hdrlen = 0; - rem = nbytes ? omin(nbytes, obj_size - offset) : obj_size - offset; /* @@ -668,11 +653,23 @@ retry_space: SOCKBUF_UNLOCK(&so->so_snd); /* - * Reduce space in the socket buffer by the size of - * the header mbuf chain. - * hdrlen is set to 0 after the first loop. + * At the beginning of the first loop check if any headers + * are specified and copy them into mbufs. Reduce space in + * the socket buffer by the size of the header mbuf chain. + * Clear hdr_uio here and hdrlen at the end of the first loop. */ - space -= hdrlen; + if (hdr_uio != NULL && hdr_uio->uio_resid > 0) { + hdr_uio->uio_td = td; + hdr_uio->uio_rw = UIO_WRITE; + /* QQQGL remove counter */ + if (space < hdr_uio->uio_resid) + counter_u64_add(sf_long_headers, 1); + hdr_uio->uio_resid = min(hdr_uio->uio_resid, space); + mh = m_uiotombuf(hdr_uio, M_WAITOK, 0, 0, 0); + hdrlen = m_length(mh, &mhtail); + space -= hdrlen; + hdr_uio = NULL; + } if (vp != NULL) { error = vn_lock(vp, LK_SHARED); @@ -944,6 +941,19 @@ sendfile(struct thread *td, struct sendfile_args * &hdr_uio); if (error != 0) goto out; +#ifdef COMPAT_FREEBSD4 + /* + * In FreeBSD < 5.0 the nbytes to send also included + * the header. If compat is specified subtract the + * header size from nbytes. + */ + if (compat) { + if (uap->nbytes > hdr_uio->uio_resid) + uap->nbytes -= hdr_uio->uio_resid; + else + uap->nbytes = 0; + } +#endif } if (hdtr.trailers != NULL) { error = copyinuio(hdtr.trailers, hdtr.trl_cnt, @@ -965,7 +975,7 @@ sendfile(struct thread *td, struct sendfile_args * } error = fo_sendfile(fp, uap->s, hdr_uio, trl_uio, uap->offset, - uap->nbytes, &sbytes, uap->flags, compat ? SFK_COMPAT : 0, td); + uap->nbytes, &sbytes, uap->flags, td); fdrop(fp, td); if (uap->sbytes != NULL) Index: sys/sys/file.h =================================================================== --- sys/sys/file.h (revision 297366) +++ sys/sys/file.h (working copy) @@ -112,7 +112,7 @@ typedef int fo_chown_t(struct file *fp, uid_t uid, struct ucred *active_cred, struct thread *td); typedef int fo_sendfile_t(struct file *fp, int sockfd, struct uio *hdr_uio, struct uio *trl_uio, off_t offset, size_t nbytes, - off_t *sent, int flags, int kflags, struct thread *td); + off_t *sent, int flags, struct thread *td); typedef int fo_seek_t(struct file *fp, off_t offset, int whence, struct thread *td); typedef int fo_fill_kinfo_t(struct file *fp, struct kinfo_file *kif, @@ -376,11 +376,11 @@ fo_chown(struct file *fp, uid_t uid, gid_t gid, st static __inline int fo_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio, struct uio *trl_uio, off_t offset, size_t nbytes, off_t *sent, int flags, - int kflags, struct thread *td) + struct thread *td) { return ((*fp->f_ops->fo_sendfile)(fp, sockfd, hdr_uio, trl_uio, offset, - nbytes, sent, flags, kflags, td)); + nbytes, sent, flags, td)); } static __inline int Index: sys/sys/socket.h =================================================================== --- sys/sys/socket.h (revision 297366) +++ sys/sys/socket.h (working copy) @@ -594,7 +594,6 @@ struct sf_hdtr { #define SF_FLAGS(rh, flags) (((rh) << 16) | (flags)) #ifdef _KERNEL -#define SFK_COMPAT 0x00000001 #define SF_READAHEAD(flags) ((flags) >> 16) #endif /* _KERNEL */ --4ndw/alBWmZEhfcZ--