Re: [dpdk-dev] packet data access bug in bpf and pdump libs

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 9 Oct 2019 17:20:58 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, October 9, 2019 5:15 PM
> > 
> > On Wed, 9 Oct 2019 17:06:24 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Wednesday, October 9, 2019 5:02 PM
> > > >
> > > > On Wed, 9 Oct 2019 11:11:46 +0000
> > > > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> > > >  
> > > > > Hi Morten,
> > > > >  
> > > > > >
> > > > > > Hi Konstantin and Stephen,
> > > > > >
> > > > > > I just noticed the same bug in your bpf and pcap libraries:
> > > > > >
> > > > > > You are using rte_pktmbuf_mtod(), but should be using  
> > > > rte_pktmbuf_read(). Otherwise you cannot read data across multiple
> > > > segments.  
> > > > >
> > > > > In plain data buffer mode expected input for BPF program is start  
> > of  
> > > > first segment packet data.  
> > > > > Other segments are simply not available to BPF program in that  
> > mode.  
> > > > > AFAIK, cBPF uses the same model.
> > > > >  
> > > > > >
> > > > > >
> > > > > > Med venlig hilsen / kind regards
> > > > > > - Morten Brørup  
> > > > >  
> > > >
> > > > For packet capture, the BPF program is only allowed to look at  
> > first  
> > > > segment.
> > > > pktmbuf_read is expensive and can cause a copy.  
> > >
> > > It is only expensive if going beyond the first segment:
> > >
> > > static inline const void *rte_pktmbuf_read(const struct rte_mbuf *m,
> > > 	uint32_t off, uint32_t len, void *buf)
> > > {
> > > 	if (likely(off + len <= rte_pktmbuf_data_len(m)))
> > > 		return rte_pktmbuf_mtod_offset(m, char *, off);
> > > 	else
> > > 		return __rte_pktmbuf_read(m, off, len, buf);
> > > }  
> > 
> > But it would mean potentially big buffer on the stack (in case)  
> 
> No, the buffer only needs to be the size of the accessed data. I use it like this:
> 
> char buffer[sizeof(uint32_t)];
> 
> for (;; pc++) {
>     switch (pc->code) {
>         case BPF_LD_ABS_32:
>             p = rte_pktmbuf_read(m, pc->k, sizeof(uint32_t), buffer);
>             if (unlikely(p == NULL)) return 0; /* Attempting to read beyond packet. Bail out. */
>             a = rte_be_to_cpu_32(*(const uint32_t *)p);
>             continue;
>         case BPF_LD_ABS_16:
>             p = rte_pktmbuf_read(m, pc->k, sizeof(uint16_t), buffer);
>             if (unlikely(p == NULL)) return 0; /* Attempting to read beyond packet. Bail out. */
>             a = rte_be_to_cpu_16(*(const uint16_t *)p);
>             continue;
> 

Reading down the chain of mbuf segments to find a uint32_t (and that potentially crosses)
seems like a waste.

The purpose of the filter is to look at packet headers. Any driver making mbufs that
are dripples of data is broken. chaining is really meant for case of jumbo or tso.