[ovs-dev] [PATCH] datapath: Avoid null deref when GSO is for verifying header integrity only.

Ben Pfaff blp at nicira.com
Mon Jan 21 22:48:33 UTC 2013


On Mon, Jan 21, 2013 at 02:25:34PM -0800, Jesse Gross wrote:
> On Mon, Jan 21, 2013 at 10:11 AM, Ben Pfaff <blp at nicira.com> wrote:
> > skb_gso_segment() has the following comment:
> >
> >  *    It may return NULL if the skb requires no segmentation.  This is
> >  *    only possible when GSO is used for verifying header integrity.
> >
> > Somehow queue_gso_packets() has never hit this case before, but some
> > failures have suddenly been reported.  This commit should fix the problem.
> >
> > Bug #14772.
> > Reported-by: Deepesh Govindan <dgovindan at vmware.com>.
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> 
> Acked-by: Jesse Gross <jesse at nicira.com>

Thanks.

> However, I think we also potentially have a similar problem in
> tunnel.c:handle_offloads().

May I leave the honors on that one to you?

> > ---
> > I don't know how to trigger this condition, so I haven't tested this
> > patch beyond verifying that it compiles against the Linux 3.2 kernel
> > on which the problem was reported.  The patch was generated against
> > branch-1.7 because that's where the problem occurred; it looks like
> > it should apply cleanly against master also.
> 
> We shouldn't normally be hitting this case because we're actually
> trying to do GSO, not header validation.  However, I guess the
> guest/backend must be generating a packet with an MSS, which tricks us
> into thinking that it's GSO, but no GSO is actually requested.  In the
> case of the bridge, header validation does take place so the situation
> is handled already.  It seems not ideal that the network backend
> doesn't sanitize these packets but it's probably good that we handle
> it in any case.

I added the comments above to the commit message for future reference.

> Our behavior here hasn't changed in a long time, I think this should
> apply equally to essentially all branches.

I applied this to every branch under the sun.  Only necessary change was
to remove the "net" argument for branch-1.4 and branch-1.5.

Thanks for the review!



More information about the dev mailing list