On Thu, Sep 23, 2010 at 11:12:48AM +0200, Mike Belopuhov wrote:
> On Thu, Sep 23, 2010 at 09:57 +0200, Bret S. Lambert wrote:
> > No objection in principle (consolidation of code in mbufs is
> > one of my currently-stalled-by-dayjob projects); comment inline.
> >
> > >
> > > OK?
> > >
> > > Index: netinet/ip_esp.c
> > > ===================================================================
> > > RCS file: /home/cvs/src/sys/netinet/ip_esp.c,v
> > > retrieving revision 1.112
> > > diff -u -p -r1.112 ip_esp.c
> > > --- netinet/ip_esp.c 22 Sep 2010 13:40:05 -0000 1.112
> > > +++ netinet/ip_esp.c 22 Sep 2010 15:47:38 -0000
> > > @@ -932,9 +932,10 @@ esp_output(struct mbuf *m, struct tdb *t
> > > * Add padding -- better to do it ourselves than use the crypto engine,
> > > * although if/when we support compression, we'd have to do that.
> > > */
> > > - pad = (u_char *) m_pad(m, padding + alen);
> > > + mo = m_inject(m, m->m_pkthdr.len, padding + alen, M_DONTWAIT);
> > > + pad = mtod(mo, u_char *);
> >
> > Sure, but this code creates a NULL pointer dereference;
> > mtod() isn't a function that does anything smart, as
> > is evidenced by the explanation in the mbuf man page:
> >
> > #define mtod(m,t) ((t)((m)->m_data))
> >
>
> /* Yeurk! */
>
> new diff. thanks for looking through!
It looks sane enough to me, although you might also take a look
at m_pulldown to ensure X contiguous bytes at offset Y in an mbuf
chain, as, IIRC, that has the possibility of not requiring a new
mbuf allocation if there truly is enough space in an existing mbuf.
But, it's your bikeshed, you can choose whether it's grape- or
strawberry-flavored.
>
> Index: netinet/ip_esp.c
> ===================================================================
> RCS file: /home/cvs/src/sys/netinet/ip_esp.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 ip_esp.c
> --- netinet/ip_esp.c 22 Sep 2010 13:40:05 -0000 1.112
> +++ netinet/ip_esp.c 23 Sep 2010 09:05:40 -0000
> @@ -932,12 +932,13 @@ esp_output(struct mbuf *m, struct tdb *t
> * Add padding -- better to do it ourselves than use the crypto engine,
> * although if/when we support compression, we'd have to do that.
> */
> - pad = (u_char *) m_pad(m, padding + alen);
> - if (pad == NULL) {
> - DPRINTF(("esp_output(): m_pad() failed for SA %s/%08x\n",
> + mo = m_inject(m, m->m_pkthdr.len, padding + alen, M_DONTWAIT);
> + if (mo == NULL) {
> + DPRINTF(("esp_output(): m_inject failed for SA %s/%08x\n",
> ipsp_address(tdb->tdb_dst), ntohl(tdb->tdb_spi)));
> return ENOBUFS;
> }
> + pad = mtod(mo, u_char *);
>
> /* Self-describing or random padding ? */
> if (!(tdb->tdb_flags & TDBF_RANDOMPADDING))
> @@ -1178,77 +1179,4 @@ checkreplaywindow32(u_int32_t seq, u_int
>
> *bitmap |= (((u_int32_t) 1) << diff);
> return 0;
> -}
> -
> -/*
> - * m_pad(m, n) pads <m> with <n> bytes at the end. The packet header
> - * length is updated, and a pointer to the first byte of the padding
> - * (which is guaranteed to be all in one mbuf) is returned.
> - */
> -
> -caddr_t
> -m_pad(struct mbuf *m, int n)
> -{
> - struct mbuf *m0, *m1;
> - int len, pad;
> - caddr_t retval;
> -
> - if (n <= 0) { /* No stupid arguments. */
> - DPRINTF(("m_pad(): pad length invalid (%d)\n", n));
> - m_freem(m);
> - return NULL;
> - }
> -
> - len = m->m_pkthdr.len;
> - pad = n;
> - m0 = m;
> -
> - while (m0->m_len < len) {
> - len -= m0->m_len;
> - m0 = m0->m_next;
> - }
> -
> - if (m0->m_len != len) {
> - DPRINTF(("m_pad(): length mismatch (should be %d instead of "
> - "%d)\n", m->m_pkthdr.len,
> - m->m_pkthdr.len + m0->m_len - len));
> -
> - m_freem(m);
> - return NULL;
> - }
> -
> - /* Check for zero-length trailing mbufs, and find the last one. */
> - for (m1 = m0; m1->m_next; m1 = m1->m_next) {
> - if (m1->m_next->m_len != 0) {
> - DPRINTF(("m_pad(): length mismatch (should be %d "
> - "instead of %d)\n", m->m_pkthdr.len,
> - m->m_pkthdr.len + m1->m_next->m_len));
> -
> - m_freem(m);
> - return NULL;
> - }
> -
> - m0 = m1->m_next;
> - }
> -
> - if ((m0->m_flags & M_EXT) ||
> - m0->m_data + m0->m_len + pad >= &(m0->m_dat[MLEN])) {
> - /* Add an mbuf to the chain. */
> - MGET(m1, M_DONTWAIT, MT_DATA);
> - if (m1 == 0) {
> - m_freem(m0);
> - DPRINTF(("m_pad(): cannot append\n"));
> - return NULL;
> - }
> -
> - m0->m_next = m1;
> - m0 = m1;
> - m0->m_len = 0;
> - }
> -
> - retval = m0->m_data + m0->m_len;
> - m0->m_len += pad;
> - m->m_pkthdr.len += pad;
> -
> - return retval;
> }
> Index: netinet/ip_ipsp.h
> ===================================================================
> RCS file: /home/cvs/src/sys/netinet/ip_ipsp.h,v
> retrieving revision 1.144
> diff -u -p -r1.144 ip_ipsp.h
> --- netinet/ip_ipsp.h 9 Jul 2010 16:58:06 -0000 1.144
> +++ netinet/ip_ipsp.h 22 Sep 2010 15:49:32 -0000
> @@ -624,9 +624,6 @@ extern int tcp_signature_tdb_input(struc
> extern int tcp_signature_tdb_output(struct mbuf *, struct tdb *,
> struct mbuf **, int, int);
>
> -/* Padding */
> -extern caddr_t m_pad(struct mbuf *, int);
> -
> /* Replay window */
> extern int checkreplaywindow32(u_int32_t, u_int32_t, u_int32_t *, u_int32_t,
> u_int32_t *, int);
