Date: Mon, 28 Mar 2016 17:26:39 -0700 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Vitalij Satanivskij <satan@ukr.net> 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> In-Reply-To: <20160323235925.GT2616@FreeBSD.org> References: <20160304124053.GA25071@hell.ukr.net> <20160323181131.GN2616@FreeBSD.org> <20160323235925.GT2616@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160329002639.GD2616>