On Mon, Oct 24, 2016 at 09:56:13AM +0200, Rafael Zalamena wrote:
> tun(4)/tap(4) function tun_dev_write() is checking for the wrong size for
> the mbuf packet header. We must check against MHLEN (the mbuf header data
> storage size) and not MINCLSIZE (smallest amount of data of a cluster).
>
> For the curious:
> MGETHDR() calls m_gethdr() which uses mbpool to get the mbuf data
> storage. mbpool is initialized at mbinit() which sets the members
> allocation size to MSIZE.
>
> - MSIZE is "256";
> - MLEN is "(MSIZE - sizeof(struct m_hdr))"
> - MHLEN is "(MLEN - sizeof(pkthdr))";
> - MINCLSIZE is "MHLEN + MLEN + 1";
>
> ok?
No, the current code is correct. At least there is no problem with it.
The idea is that for up to MINCLSIZE bytes no cluster is allocated and
instead up to 2 mbufs are connected. This was an optimisation because
allocating clusters was hard many years ago. It could well be that this no
longer makes sense but in that case it would make more sense to adjust
MINCLSIZE and get the benefit on all loops building mbuf chains at the
same time.
>
> Index: sys/net/if_tun.c
> ===================================================================
> RCS file: /home/obsdcvs/src/sys/net/if_tun.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 if_tun.c
> --- sys/net/if_tun.c 4 Sep 2016 15:46:39 -0000 1.169
> +++ sys/net/if_tun.c 24 Oct 2016 07:43:03 -0000
> @@ -895,7 +895,7 @@ tun_dev_write(struct tun_softc *tp, stru
> if (m == NULL)
> return (ENOBUFS);
> mlen = MHLEN;
> - if (uio->uio_resid >= MINCLSIZE) {
> + if (uio->uio_resid >= MHLEN) {
> MCLGET(m, M_DONTWAIT);
> if (!(m->m_flags & M_EXT)) {
> m_free(m);
> @@ -926,7 +926,7 @@ tun_dev_write(struct tun_softc *tp, stru
> break;
> }
> mlen = MLEN;
> - if (uio->uio_resid >= MINCLSIZE) {
> + if (uio->uio_resid >= MHLEN) {
> MCLGET(m, M_DONTWAIT);
> if (!(m->m_flags & M_EXT)) {
> error = ENOBUFS;
>
--
:wq Claudio
