On Mon, Nov 14, 2011 at 03:02:16PM -0800, Jesse Gross wrote:
> On Sat, Nov 12, 2011 at 1:01 PM, Ben Pfaff <b...@nicira.com> wrote:
> > diff --git a/datapath/README b/datapath/README
> > index 2d8a141..23a5a81 100644
> > --- a/datapath/README
> > +++ b/datapath/README
> > @@ -145,6 +145,41 @@ not understand the "vlan" key will not see either of
> > those attributes
> > ??and therefore will not misinterpret them. ??(Also, the outer eth_type
> > ??is still 0x8100, not changed to 0x0800.)
> >
> > +Handling malformed packets
> > +--------------------------
> > +
> > +Don't drop packets in the kernel for malformed protocols headers, bad
>
> "protocol headers"
Fixed.
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 4d16caa..b331ab6 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -480,9 +480,12 @@ static int parse_vlan(struct sk_buff *skb, struct
> > sw_flow_key *key)
> > ?? ?? ?? ??};
> > ?? ?? ?? ??struct qtag_prefix *qp;
> >
> > + ?? ?? ?? if (unlikely(skb->len < sizeof(struct qtag_prefix) +
> > sizeof(__be16)))
> > + ?? ?? ?? ?? ?? ?? ?? return 0;
> > +
> > ?? ?? ?? ??if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??
> > sizeof(__be16))))
> > - ?? ?? ?? ?? ?? ?? ?? return -ENOMEM;
> > + ?? ?? ?? ?? ?? ?? ?? return 0;
>
> I think we want to continue returning -ENOMEM in this case because it
> actually does indicate a memory allocation failure and we should throw
> the packet away.
You're right (duh), fixed.
> > @@ -1049,14 +1052,25 @@ int flow_from_nlattrs(struct sw_flow_key *swkey,
> > int *key_lenp,
> [...]
> > + ?? ?? ?? ?? ?? ?? ?? if (tci & htons(VLAN_TAG_PRESENT)) {
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? swkey->eth.tci = tci;
> > +
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? err = parse_flow_nlattrs(encap, a,
> > &attrs);
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? if (err)
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? return err;
> > + ?? ?? ?? ?? ?? ?? ?? } else if (!tci) {
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? /* Used for a truncated 802.1Q header. */
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? if (nla_len(encap))
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? return -EINVAL;
> > +
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? swkey->eth.type = htons(ETH_P_8021Q);
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? *key_lenp = key_len;
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? return 0;
> > + ?? ?? ?? ?? ?? ?? ?? } else
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? return -EINVAL;
>
> Another one of these one-armed things.
Fixed.
> Also, I think that we should enforce that if there is an EtherType of
> 0x8100 then there must be a vlan attribute (and the same thing in
> userspace).
That's what I thought I was doing but as you say I did it wrong.
OK, here's an incremental for that part. There's a lot of irrelevant
churn not shown here due to the addition of ovs_action_push_vlan in
revising patch 2, so I'll send a new copy of the patch separately.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
