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);

Reply via email to