On Fri, Apr 20, 2012 at 10:58:43AM -0700, Jeff Kirsher wrote: > On Fri, 2012-04-20 at 13:46 -0400, Neil Horman wrote: > > On Fri, Apr 20, 2012 at 10:11:37AM -0700, Jeff Kirsher wrote: > > > From: Alexander Duyck <alexander.h.du...@intel.com> > > > > > > The fcoe_recv_frame path makes a call to skb_linearize without verifying > > > that it has actually linearized the frame. This can lead to memory > > > corruption issues if we use the frame assuming it is 3K, instead of a > > > smaller 512 or 1K sized frame. > > > > > > In order to ensure that the linearize call result is tested I have moved > > > it > > > into the location that was previously taken in fcoe_rcv which was > > > verifying > > > we had at least the header. This way instead of breaking things up into > > > two calls to __pskb_pull_tail we rip the bandage off in one tear and just > > > take care of all of the linearization in a single call. > > > > > > A better fix would be to have the FCoE path handle non-linear sk_buffs, > > > but > > > I am not confident enough in my understanding of FCoE to rewrite it to > > > handle it. I will leave that to the FCoE maintainers to handle. > > > > > > Signed-off-by: Alexander Duyck <alexander.h.du...@intel.com> > > > Tested-by: Ross Brattain <ross.b.bratt...@intel.com> > > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com> > > > > > > --- > > > drivers/scsi/fcoe/fcoe.c | 8 ++++---- > > > 1 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > > > index e959960..5d37ce6 100644 > > > --- a/drivers/scsi/fcoe/fcoe.c > > > +++ b/drivers/scsi/fcoe/fcoe.c > > > @@ -1336,11 +1336,12 @@ static int fcoe_rcv(struct sk_buff *skb, struct > > > net_device *netdev, > > > } > > > > > > /* > > > - * Check for minimum frame length, and make sure required FCoE > > > + * Check for minimum frame length, and make sure FCoE frame > > > * and FC headers are pulled into the linear data area. > > > + * > > > + * This is not ideal, and FCoE should learn to handle non-linear frames > > > */ > > > - if (unlikely((skb->len < FCOE_MIN_FRAME) || > > > - !pskb_may_pull(skb, FCOE_HEADER_LEN))) > > > + if (unlikely((skb->len < FCOE_MIN_FRAME) || skb_linearize(skb))) > > > goto err; > > > > > this moves the call to skb_linearize into softirq context however. Is that > > safe > > to do? I can't find any other code point that does this, so I can't say for > > certain. It looks ok, but I'm not sure. > > > > Also, this is going to increase softirq execution time. FCOE_MIN_FRAME is > > defined as 38 bytes, so it seems to me that that the pskb_may_pull will > > likely > > return very quickly, whereas the the linearize call has lots of potential to > > take a long time. > > I personally cannot say. I know that Ross (and team) tested this patch > thoroughly with no issues. Ross can respond to the performance that he > or the team experienced due to this patch. Alex had a good > point/argument to make this change, so I will let Alex respond on the > rest of your points Neil. > Copy that, thanks Jeff! Neil
> > > > Might it be better to insead, just reduce fcoe_rcv to a minimum effort > > routine > > that parses out the fh_ox_id value from the skb to determine the > > destination cpu > > of the frame? Then we can move all the other validation work that fcoe_rcv > > does > > up to fcoe_recv_frame, where we run in process context? I like the idea of > > not > > having to do two pulls/linearizations, but I think it might be better to do > > it > > out of the softirq handler if we can. > > > > Regards > > Neil > > > skb_set_transport_header(skb, sizeof(struct fcoe_hdr)); > > > @@ -1676,7 +1677,6 @@ static void fcoe_recv_frame(struct sk_buff *skb) > > > skb->dev ? skb->dev->name : "<NULL>"); > > > > > > port = lport_priv(lport); > > > - skb_linearize(skb); /* check for skb_is_nonlinear is within > > > skb_linearize */ > > > > > > /* > > > * Frame length checks and setting up the header pointers > > > _______________________________________________ > > > devel mailing list > > > devel@open-fcoe.org > > > https://lists.open-fcoe.org/mailman/listinfo/devel > > > > > _______________________________________________ devel mailing list devel@open-fcoe.org https://lists.open-fcoe.org/mailman/listinfo/devel
