Bug in sbsndptr()

[Andre Oppermann]

On 26.02.2013 14:38, Lawrence Stewart wrote:
> Hi Andre,

Hi Lawrence, :-)

> A colleague and I spent a very frustrating day tracing an accounting bug
> in the multipath TCP patch we're working on at CAIA to a bug in
> sbsndptr(). I haven't tested it with regular TCP yet, but I believe the
> following patch fixes the bug (proposed commit log message is at the top
> of the patch):
>
> http://people.freebsd.org/~lstewart/patches/misctcp/sbsndptr_mnext_10.x.r247314.diff
>
> The patch should have no tangible effect to operation other than to
> ensure the function delivers on the promise to return the closest mbuf
> in the chain for the given offset.

I agree that the description of sbsndptr() can be misleading as it refers
to the point in time when the pointer was updated last.  Relative to now
the real offset may be at the beginning of the next mbuf.

As you note in the proposed commit message by the time the send pointer
is calculated we may have reached the end of the chain and must avoid
storing a NULL pointer.  The mbuf copy routines simply skips over the
additional mbuf in the chain using the returned offset.

I wonder how this has caused trouble with your multipath patch.  You'd
have to copy the sockbuf contents as well and unless you're using custom
sockbuf and mbuf chain functions this shouldn't be a problem.  Using
custom functions on a socket buffer is a delicate approach.  For a sockbuf
consumer being able to handle valid offsets into an mbuf chain is a core
feature and must-have part of the functionality.

> I would appreciate a review and any thoughts.

I think you have found a valid (micro-)optimization.  However you're
still making a dangerous assumption in that the next mbuf is indeed
the one you want.  This may not be true in subtle ways when the chain
contains m_len=0 mbufs in it.  I'm not aware of it actually happening
but it can't be ruled out either if custom sockbuf manipulation functions
are in use.

I'd recommend the following:
have you custom sockbuf function handle forward seeking like the other
m_copy() functions; and/or apply a patch along the (untested) example
below.

Cheers
-- 
Andre

Index: uipc_sockbuf.c
===================================================================
--- uipc_sockbuf.c      (revision 247775)
+++ uipc_sockbuf.c      (working copy)
@@ -936,10 +936,17 @@
                 return (sb->sb_mb);
         }

-       /* Return closest mbuf in chain for current offset. */
+       /* Return closest known mbuf in chain for current offset. */
         *moff = off - sb->sb_sndptroff;
         m = ret = sb->sb_sndptr ? sb->sb_sndptr : sb->sb_mb;

+       /* Possibly seek forward to return the closest mbuf to the offset. */
+       while (*moff >= m->m_len && ret->m_next != NULL) {
+               *moff -= m->m_len;
+               ret = m->m_next;
+       }
+       KASSERT(*moff != NULL, ("%s: moff is NULL", __func__));
+
         /* Advance by len to be as close as possible for the next transmit. */
         for (off = off - sb->sb_sndptroff + len - 1;
              off > 0 && m != NULL && off >= m->m_len;