| Message ID | 1381384296-1821-1-git-send-email-fan.du@windriver.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Oct 10, 2013 at 01:51:36PM +0800, Fan Du wrote: > igb/ixgbe have hardware sctp checksum support, when this feature is enabled > and also IPsec is armed to protect sctp traffic, ugly things happened as > xfrm_output checks CHECKSUM_PARTIAL to do check sum operation(sum every thing > up and pack the 16bits result in the checksum field). The result is fail > establishment of sctp communication. > Shouldn't this be fixed in the xfrm code then? E.g. check the device features for SCTP checksum offloading and and skip the checksum during xfrm output if its available? Or am I missing something? Neil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/10/2013 01:51 AM, Fan Du wrote: > igb/ixgbe have hardware sctp checksum support, when this feature is enabled > and also IPsec is armed to protect sctp traffic, ugly things happened as > xfrm_output checks CHECKSUM_PARTIAL to do check sum operation(sum every thing > up and pack the 16bits result in the checksum field). The result is fail > establishment of sctp communication. > > And I saw another point in this part of code, when IPsec is not armed, > sctp communication is good, however setting setting CHECKSUM_PARTIAL will > make xfrm_output compute dummy checksum values which will be overwritten by > hardware lately. > > So this patch try to solve above two issues together. > > Signed-off-by: Fan Du <fan.du@windriver.com> > --- > note: > igb/ixgbe hardware is not handy on my side, so just build test only. > > --- > net/sctp/output.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/net/sctp/output.c b/net/sctp/output.c > index 0ac3a65..f0b9cc5 100644 > --- a/net/sctp/output.c > +++ b/net/sctp/output.c > @@ -372,6 +372,16 @@ static void sctp_packet_set_owner_w(struct sk_buff *skb, struct sock *sk) > atomic_inc(&sk->sk_wmem_alloc); > } > > +static int is_xfrm_armed(struct dst_entry *dst) > +{ > +#ifdef CONFIG_XFRM > + /* If dst->xfrm is valid, this skb needs to be transformed */ > + return dst->xfrm != NULL; > +#else > + return 0; > +#endif > +} > + > /* All packets are sent to the network through this function from > * sctp_outq_tail(). > * > @@ -536,20 +546,21 @@ int sctp_packet_transmit(struct sctp_packet *packet) > * by CRC32-C as described in <draft-ietf-tsvwg-sctpcsum-02.txt>. > */ > if (!sctp_checksum_disable) { > - if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) { > + if ((!(dst->dev->features & NETIF_F_SCTP_CSUM)) || > + is_xfrm_armed(dst)) { > + > __u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len); > > /* 3) Put the resultant value into the checksum field in the > * common header, and leave the rest of the bits unchanged. > */ > sh->checksum = sctp_end_cksum(crc32); > - } else { > - /* no need to seed pseudo checksum for SCTP */ > - nskb->ip_summed = CHECKSUM_PARTIAL; > - nskb->csum_start = (skb_transport_header(nskb) - > - nskb->head); > - nskb->csum_offset = offsetof(struct sctphdr, checksum); > - } > + } else > + /* Mark skb as CHECKSUM_UNNECESSARY to let hardware compute > + * the checksum, and also avoid xfrm_output to do unceccessary > + * checksum. > + */ > + nskb->ip_summed = CHECKSUM_UNNECESSARY; > } In addition to what Niel said, the use of CHECKSUM_UNNECESSARY is incorrect as it will cause the nic to not compute the checksum. The checksum offload depends on the use of CHECKSUM_PARTIAL. -vlad > > /* IP layer ECN support > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013年10月10日 21:11, Neil Horman wrote: > On Thu, Oct 10, 2013 at 01:51:36PM +0800, Fan Du wrote: >> igb/ixgbe have hardware sctp checksum support, when this feature is enabled >> and also IPsec is armed to protect sctp traffic, ugly things happened as >> xfrm_output checks CHECKSUM_PARTIAL to do check sum operation(sum every thing >> up and pack the 16bits result in the checksum field). The result is fail >> establishment of sctp communication. >> > Shouldn't this be fixed in the xfrm code then? E.g. check the device features > for SCTP checksum offloading and and skip the checksum during xfrm output if its > available? sigh... same as my first thought. However why should xfrm_output check whether the skb is a SCTP one as well as whether the associated dev is able to do hw SCTP crc32-c checksum in the first place, and then call SCTP crc checksum value API to write the correct checksum *after* this skb has leaven SCTP layer??? The checksum operation in xfrm_ouput fits TCP/UDP/IP layer checksum semantics, e.g. calculate 16bits value by sum almost everything up. Make a SCTP specific exception in this common path sound wired, as the fix can be done in SCTP codes easily with minimum changes, so not any bit of consequence for non-SCTP traffic. And If you/Vlad insist to do so as well as no objection from Steffen/David, I'm happy to send this revised version as your suggested. Anyway, I should have separated this patch for two which makes the issues more clear for you and Vlad to review. > Or am I missing something? > Neil > >
On 2013年10月10日 22:11, Vlad Yasevich wrote: > On 10/10/2013 01:51 AM, Fan Du wrote: >> igb/ixgbe have hardware sctp checksum support, when this feature is enabled >> and also IPsec is armed to protect sctp traffic, ugly things happened as >> xfrm_output checks CHECKSUM_PARTIAL to do check sum operation(sum every thing >> up and pack the 16bits result in the checksum field). The result is fail >> establishment of sctp communication. >> >> And I saw another point in this part of code, when IPsec is not armed, >> sctp communication is good, however setting setting CHECKSUM_PARTIAL will >> make xfrm_output compute dummy checksum values which will be overwritten by >> hardware lately. >> >> So this patch try to solve above two issues together. >> >> Signed-off-by: Fan Du <fan.du@windriver.com> >> --- >> note: >> igb/ixgbe hardware is not handy on my side, so just build test only. >> >> --- >> net/sctp/output.c | 27 +++++++++++++++++++-------- >> 1 file changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/net/sctp/output.c b/net/sctp/output.c >> index 0ac3a65..f0b9cc5 100644 >> --- a/net/sctp/output.c >> +++ b/net/sctp/output.c >> @@ -372,6 +372,16 @@ static void sctp_packet_set_owner_w(struct sk_buff *skb, struct sock *sk) >> atomic_inc(&sk->sk_wmem_alloc); >> } >> >> +static int is_xfrm_armed(struct dst_entry *dst) >> +{ >> +#ifdef CONFIG_XFRM >> + /* If dst->xfrm is valid, this skb needs to be transformed */ >> + return dst->xfrm != NULL; >> +#else >> + return 0; >> +#endif >> +} >> + >> /* All packets are sent to the network through this function from >> * sctp_outq_tail(). >> * >> @@ -536,20 +546,21 @@ int sctp_packet_transmit(struct sctp_packet *packet) >> * by CRC32-C as described in <draft-ietf-tsvwg-sctpcsum-02.txt>. >> */ >> if (!sctp_checksum_disable) { >> - if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) { >> + if ((!(dst->dev->features & NETIF_F_SCTP_CSUM)) || >> + is_xfrm_armed(dst)) { >> + >> __u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len); >> >> /* 3) Put the resultant value into the checksum field in the >> * common header, and leave the rest of the bits unchanged. >> */ >> sh->checksum = sctp_end_cksum(crc32); >> - } else { >> - /* no need to seed pseudo checksum for SCTP */ >> - nskb->ip_summed = CHECKSUM_PARTIAL; >> - nskb->csum_start = (skb_transport_header(nskb) - >> - nskb->head); >> - nskb->csum_offset = offsetof(struct sctphdr, checksum); >> - } >> + } else >> + /* Mark skb as CHECKSUM_UNNECESSARY to let hardware compute >> + * the checksum, and also avoid xfrm_output to do unceccessary >> + * checksum. >> + */ >> + nskb->ip_summed = CHECKSUM_UNNECESSARY; >> } > > In addition to what Niel said, the use of CHECKSUM_UNNECESSARY is > incorrect as it will cause the nic to not compute the checksum. > The checksum offload depends on the use of CHECKSUM_PARTIAL. My bad, my head is cramed with IPsec recently :( We should fix this in ipv4/v6 output path. > -vlad > > >> >> /* IP layer ECN support >> > >
diff --git a/net/sctp/output.c b/net/sctp/output.c index 0ac3a65..f0b9cc5 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -372,6 +372,16 @@ static void sctp_packet_set_owner_w(struct sk_buff *skb, struct sock *sk) atomic_inc(&sk->sk_wmem_alloc); } +static int is_xfrm_armed(struct dst_entry *dst) +{ +#ifdef CONFIG_XFRM + /* If dst->xfrm is valid, this skb needs to be transformed */ + return dst->xfrm != NULL; +#else + return 0; +#endif +} + /* All packets are sent to the network through this function from * sctp_outq_tail(). * @@ -536,20 +546,21 @@ int sctp_packet_transmit(struct sctp_packet *packet) * by CRC32-C as described in <draft-ietf-tsvwg-sctpcsum-02.txt>. */ if (!sctp_checksum_disable) { - if (!(dst->dev->features & NETIF_F_SCTP_CSUM)) { + if ((!(dst->dev->features & NETIF_F_SCTP_CSUM)) || + is_xfrm_armed(dst)) { + __u32 crc32 = sctp_start_cksum((__u8 *)sh, cksum_buf_len); /* 3) Put the resultant value into the checksum field in the * common header, and leave the rest of the bits unchanged. */ sh->checksum = sctp_end_cksum(crc32); - } else { - /* no need to seed pseudo checksum for SCTP */ - nskb->ip_summed = CHECKSUM_PARTIAL; - nskb->csum_start = (skb_transport_header(nskb) - - nskb->head); - nskb->csum_offset = offsetof(struct sctphdr, checksum); - } + } else + /* Mark skb as CHECKSUM_UNNECESSARY to let hardware compute + * the checksum, and also avoid xfrm_output to do unceccessary + * checksum. + */ + nskb->ip_summed = CHECKSUM_UNNECESSARY; } /* IP layer ECN support
igb/ixgbe have hardware sctp checksum support, when this feature is enabled and also IPsec is armed to protect sctp traffic, ugly things happened as xfrm_output checks CHECKSUM_PARTIAL to do check sum operation(sum every thing up and pack the 16bits result in the checksum field). The result is fail establishment of sctp communication. And I saw another point in this part of code, when IPsec is not armed, sctp communication is good, however setting setting CHECKSUM_PARTIAL will make xfrm_output compute dummy checksum values which will be overwritten by hardware lately. So this patch try to solve above two issues together. Signed-off-by: Fan Du <fan.du@windriver.com> --- note: igb/ixgbe hardware is not handy on my side, so just build test only. --- net/sctp/output.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)