Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Tom Herbert]

On Tue, Oct 27, 2015 at 8:02 AM, Hannes Frederic Sowa
 wrote:
> We cannot reliable calculate packet size on MSG_MORE corked sockets
> and thus cannot decide if they are going to be fragmented later on,
> so better not use CHECKSUM_PARTIAL in the first place.
>
> The IPv6 code also intended to protect and not use CHECKSUM_PARTIAL in
> the existence of IPv6 extension headers, but the condition was wrong. Fix
> it up, too. Also the condition to check whether the packet fits into
> one fragment was wrong and has been corrected.
>
> Fixes: commit 32dce968dd987 ("ipv6: Allow for partial checksums on non-ufo 
> packets")
> See-also: commit 72e843bb09d45 ("ipv6: ip6_fragment() should check 
> CHECKSUM_PARTIAL")
> Cc: Eric Dumazet 
> Cc: Vlad Yasevich 
> Cc: Benjamin Coddington 
> Cc: Tom Herbert 
> Signed-off-by: Hannes Frederic Sowa 
> ---
>  net/ipv6/ip6_output.c | 70 
> ---
>  1 file changed, 33 insertions(+), 37 deletions(-)
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index c265068..9828a71 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1272,6 +1272,7 @@ static int __ip6_append_data(struct sock *sk,
> struct rt6_info *rt = (struct rt6_info *)cork->dst;
> struct ipv6_txoptions *opt = v6_cork->opt;
> int csummode = CHECKSUM_NONE;
> +   unsigned int maxnonfragsize, headersize;
>
> skb = skb_peek_tail(queue);
> if (!skb) {
> @@ -1289,38 +1290,43 @@ static int __ip6_append_data(struct sock *sk,
> maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen -
>  sizeof(struct frag_hdr);
>
> -   if (mtu <= sizeof(struct ipv6hdr) + IPV6_MAXPLEN) {

It seems like most of the diffs in this patch are a result of
eliminating this line. Is this check not needed?

> -   unsigned int maxnonfragsize, headersize;
> -
> -   headersize = sizeof(struct ipv6hdr) +
> -(opt ? opt->opt_flen + opt->opt_nflen : 0) +
> -(dst_allfrag(>dst) ?
> - sizeof(struct frag_hdr) : 0) +
> -rt->rt6i_nfheader_len;
> -
> -   if (ip6_sk_ignore_df(sk))
> -   maxnonfragsize = sizeof(struct ipv6hdr) + 
> IPV6_MAXPLEN;
> -   else
> -   maxnonfragsize = mtu;
> +   headersize = sizeof(struct ipv6hdr) +
> +(opt ? opt->opt_flen + opt->opt_nflen : 0) +
> +(dst_allfrag(>dst) ?
> + sizeof(struct frag_hdr) : 0) +
> +rt->rt6i_nfheader_len;
> +
> +   if (cork->length + length > mtu - headersize && dontfrag &&
> +   (sk->sk_protocol == IPPROTO_UDP ||
> +sk->sk_protocol == IPPROTO_RAW)) {
> +   ipv6_local_rxpmtu(sk, fl6, mtu - headersize +
> +   sizeof(struct ipv6hdr));
> +   goto emsgsize;
> +   }
>
> -   /* dontfrag active */
> -   if ((cork->length + length > mtu - headersize) && dontfrag &&
> -   (sk->sk_protocol == IPPROTO_UDP ||
> -sk->sk_protocol == IPPROTO_RAW)) {
> -   ipv6_local_rxpmtu(sk, fl6, mtu - headersize +
> -  sizeof(struct ipv6hdr));
> -   goto emsgsize;
> -   }
> +   if (ip6_sk_ignore_df(sk))
> +   maxnonfragsize = sizeof(struct ipv6hdr) + IPV6_MAXPLEN;
> +   else
> +   maxnonfragsize = mtu;
>
> -   if (cork->length + length > maxnonfragsize - headersize) {
> +   if (cork->length + length > maxnonfragsize - headersize) {
>  emsgsize:
> -   ipv6_local_error(sk, EMSGSIZE, fl6,
> -mtu - headersize +
> -sizeof(struct ipv6hdr));
> -   return -EMSGSIZE;
> -   }
> +   ipv6_local_error(sk, EMSGSIZE, fl6,
> +mtu - headersize +
> +sizeof(struct ipv6hdr));
> +   return -EMSGSIZE;
> }
>
> +   /* CHECKSUM_PARTIAL only with no extension headers and when

No, please don't do this. CHECKSUM_PARTIAL should work with extension
headers as defined, so this is just disabling otherwise valid and
useful functionality. If (some) drivers have problems with this they
need to be identified and fixed.

> +* we are not going to fragment
> +*/
> +   if (transhdrlen && sk->sk_protocol == IPPROTO_UDP &&
> +   headersize == sizeof(struct ipv6hdr) &&
> +   length < mtu - headersize &&
> +   !(flags & MSG_MORE) &&
> +   

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Tom Herbert]

On Tue, Oct 27, 2015 at 11:29 AM, Hannes Frederic Sowa
 wrote:
> On Tue, Oct 27, 2015, at 18:32, Tom Herbert wrote:
>> On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
>>  wrote:
>> >
>> >
>> > On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   if
>> > (cork->length + length > maxnonfragsize - headersize) {
>> >> > +   if (cork->length + length > maxnonfragsize - headersize) {
>> >> >  emsgsize:
>> >> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
>> >> > -mtu - headersize +
>> >> > -sizeof(struct ipv6hdr));
>> >> > -   return -EMSGSIZE;
>> >> > -   }
>> >> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
>> >> > +mtu - headersize +
>> >> > +sizeof(struct ipv6hdr));
>> >> > +   return -EMSGSIZE;
>> >> > }
>> >> >
>> >> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
>> >>
>> >> No, please don't do this. CHECKSUM_PARTIAL should work with extension
>> >> headers as defined, so this is just disabling otherwise valid and
>> >> useful functionality. If (some) drivers have problems with this they
>> >> need to be identified and fixed.
>> >
>> > I don't understand. The old code already didn't allow the use of
>> > opt_flen with CHECKSUM_PARTIAL.
>> >
>> Then that's a problem with the old code :-). Is there any other reason
>> that we can't use CHECKSUM_PARTIAL with extension headers other than
>> lack of correct driver support?
>
> The lack of correct driver support is a big bumper, but as I wrote, I
> don't see a reason to not lift this restriction in net-next. I proposed
> a new feature flag, or by looking at your series, we could probably use
> the extension header okay field for that.
>
Okay, but why bother doing this for net? This problem has obviously
existed for a while, and even if the restriction is maintained here
there are still other paths that don't go through ip_append_data that
could trip the bug. Also, drivers are welcome to fix their issues in
net I believe.

> I would be conservative in net though.
>
> Bye,
> Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Tom Herbert]

On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
 wrote:
>
>
> On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   if
> (cork->length + length > maxnonfragsize - headersize) {
>> > +   if (cork->length + length > maxnonfragsize - headersize) {
>> >  emsgsize:
>> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
>> > -mtu - headersize +
>> > -sizeof(struct ipv6hdr));
>> > -   return -EMSGSIZE;
>> > -   }
>> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
>> > +mtu - headersize +
>> > +sizeof(struct ipv6hdr));
>> > +   return -EMSGSIZE;
>> > }
>> >
>> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
>>
>> No, please don't do this. CHECKSUM_PARTIAL should work with extension
>> headers as defined, so this is just disabling otherwise valid and
>> useful functionality. If (some) drivers have problems with this they
>> need to be identified and fixed.
>
> I don't understand. The old code already didn't allow the use of
> opt_flen with CHECKSUM_PARTIAL.
>
Then that's a problem with the old code :-). Is there any other reason
that we can't use CHECKSUM_PARTIAL with extension headers other than
lack of correct driver support?

> The MSG_MORE check has nothing to do with that but only with corking.
>
> Bye,
> Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Hannes Frederic Sowa]

On Tue, Oct 27, 2015, at 18:32, Tom Herbert wrote:
> On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
>  wrote:
> >
> >
> > On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   if
> > (cork->length + length > maxnonfragsize - headersize) {
> >> > +   if (cork->length + length > maxnonfragsize - headersize) {
> >> >  emsgsize:
> >> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
> >> > -mtu - headersize +
> >> > -sizeof(struct ipv6hdr));
> >> > -   return -EMSGSIZE;
> >> > -   }
> >> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
> >> > +mtu - headersize +
> >> > +sizeof(struct ipv6hdr));
> >> > +   return -EMSGSIZE;
> >> > }
> >> >
> >> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
> >>
> >> No, please don't do this. CHECKSUM_PARTIAL should work with extension
> >> headers as defined, so this is just disabling otherwise valid and
> >> useful functionality. If (some) drivers have problems with this they
> >> need to be identified and fixed.
> >
> > I don't understand. The old code already didn't allow the use of
> > opt_flen with CHECKSUM_PARTIAL.
> >
> Then that's a problem with the old code :-). Is there any other reason
> that we can't use CHECKSUM_PARTIAL with extension headers other than
> lack of correct driver support?

The lack of correct driver support is a big bumper, but as I wrote, I
don't see a reason to not lift this restriction in net-next. I proposed
a new feature flag, or by looking at your series, we could probably use
the extension header okay field for that.

I would be conservative in net though.

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Hannes Frederic Sowa]

On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   if
(cork->length + length > maxnonfragsize - headersize) {
> > +   if (cork->length + length > maxnonfragsize - headersize) {
> >  emsgsize:
> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
> > -mtu - headersize +
> > -sizeof(struct ipv6hdr));
> > -   return -EMSGSIZE;
> > -   }
> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
> > +mtu - headersize +
> > +sizeof(struct ipv6hdr));
> > +   return -EMSGSIZE;
> > }
> >
> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
> 
> No, please don't do this. CHECKSUM_PARTIAL should work with extension
> headers as defined, so this is just disabling otherwise valid and
> useful functionality. If (some) drivers have problems with this they
> need to be identified and fixed.

I don't understand. The old code already didn't allow the use of
opt_flen with CHECKSUM_PARTIAL.

The MSG_MORE check has nothing to do with that but only with corking.

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Hannes Frederic Sowa]

On Tue, Oct 27, 2015, at 19:37, Tom Herbert wrote:
> On Tue, Oct 27, 2015 at 11:29 AM, Hannes Frederic Sowa
>  wrote:
> > On Tue, Oct 27, 2015, at 18:32, Tom Herbert wrote:
> >> On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
> >>  wrote:
> >> >
> >> >
> >> > On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   if
> >> > (cork->length + length > maxnonfragsize - headersize) {
> >> >> > +   if (cork->length + length > maxnonfragsize - headersize) {
> >> >> >  emsgsize:
> >> >> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
> >> >> > -mtu - headersize +
> >> >> > -sizeof(struct ipv6hdr));
> >> >> > -   return -EMSGSIZE;
> >> >> > -   }
> >> >> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
> >> >> > +mtu - headersize +
> >> >> > +sizeof(struct ipv6hdr));
> >> >> > +   return -EMSGSIZE;
> >> >> > }
> >> >> >
> >> >> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
> >> >>
> >> >> No, please don't do this. CHECKSUM_PARTIAL should work with extension
> >> >> headers as defined, so this is just disabling otherwise valid and
> >> >> useful functionality. If (some) drivers have problems with this they
> >> >> need to be identified and fixed.
> >> >
> >> > I don't understand. The old code already didn't allow the use of
> >> > opt_flen with CHECKSUM_PARTIAL.
> >> >
> >> Then that's a problem with the old code :-). Is there any other reason
> >> that we can't use CHECKSUM_PARTIAL with extension headers other than
> >> lack of correct driver support?
> >
> > The lack of correct driver support is a big bumper, but as I wrote, I
> > don't see a reason to not lift this restriction in net-next. I proposed
> > a new feature flag, or by looking at your series, we could probably use
> > the extension header okay field for that.
> >
> Okay, but why bother doing this for net? This problem has obviously
> existed for a while, and even if the restriction is maintained here
> there are still other paths that don't go through ip_append_data that
> could trip the bug. Also, drivers are welcome to fix their issues in
> net I believe.

I even don't know if it could be a hardware issue. Also I don't want to
break people's communication with a patch.
IMHO without the WARN_ON_ONCEs, which I agreed to remove, I currently
don't see any problem for net.

You don't agree on a netdev-feature flag, indicating the driver is okay
with hardware checksumming and extension headers? We could add this to
net-next pretty fast, I think. It does not require people to revert this
patch in case their driver misbehaves and we don't get a fix for it,
soon. Also what should we do if the driver simply does not support
extension headers + checksum offloading? Completely kill checksum
offloading for IPv6?

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Tom Herbert]

On Tue, Oct 27, 2015 at 2:42 PM, Hannes Frederic Sowa
 wrote:
> Hi Tom,
>
> On Tue, Oct 27, 2015, at 20:19, Hannes Frederic Sowa wrote:
>> On Tue, Oct 27, 2015, at 19:37, Tom Herbert wrote:
>> > On Tue, Oct 27, 2015 at 11:29 AM, Hannes Frederic Sowa
>> >  wrote:
>> > > On Tue, Oct 27, 2015, at 18:32, Tom Herbert wrote:
>> > >> On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
>> > >>  wrote:
>> > >> >
>> > >> >
>> > >> > On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   
>> > >> > if
>> > >> > (cork->length + length > maxnonfragsize - headersize) {
>> > >> >> > +   if (cork->length + length > maxnonfragsize - headersize) {
>> > >> >> >  emsgsize:
>> > >> >> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
>> > >> >> > -mtu - headersize +
>> > >> >> > -sizeof(struct ipv6hdr));
>> > >> >> > -   return -EMSGSIZE;
>> > >> >> > -   }
>> > >> >> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
>> > >> >> > +mtu - headersize +
>> > >> >> > +sizeof(struct ipv6hdr));
>> > >> >> > +   return -EMSGSIZE;
>> > >> >> > }
>> > >> >> >
>> > >> >> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
>> > >> >>
>> > >> >> No, please don't do this. CHECKSUM_PARTIAL should work with extension
>> > >> >> headers as defined, so this is just disabling otherwise valid and
>> > >> >> useful functionality. If (some) drivers have problems with this they
>> > >> >> need to be identified and fixed.
>> > >> >
>> > >> > I don't understand. The old code already didn't allow the use of
>> > >> > opt_flen with CHECKSUM_PARTIAL.
>> > >> >
>> > >> Then that's a problem with the old code :-). Is there any other reason
>> > >> that we can't use CHECKSUM_PARTIAL with extension headers other than
>> > >> lack of correct driver support?
>> > >
>> > > The lack of correct driver support is a big bumper, but as I wrote, I
>> > > don't see a reason to not lift this restriction in net-next. I proposed
>> > > a new feature flag, or by looking at your series, we could probably use
>> > > the extension header okay field for that.
>> > >
>> > Okay, but why bother doing this for net? This problem has obviously
>> > existed for a while, and even if the restriction is maintained here
>> > there are still other paths that don't go through ip_append_data that
>> > could trip the bug. Also, drivers are welcome to fix their issues in
>> > net I believe.
>>
>> I even don't know if it could be a hardware issue. Also I don't want to
>> break people's communication with a patch.
>> IMHO without the WARN_ON_ONCEs, which I agreed to remove, I currently
>> don't see any problem for net.
>>
>> You don't agree on a netdev-feature flag, indicating the driver is okay
>> with hardware checksumming and extension headers? We could add this to
>> net-next pretty fast, I think. It does not require people to revert this
>> patch in case their driver misbehaves and we don't get a fix for it,
>> soon. Also what should we do if the driver simply does not support
>> extension headers + checksum offloading? Completely kill checksum
>> offloading for IPv6?
>
> I posted v3 just now. I would like to let David consider it for net
> inclusion. We can work on how to lift this limitation then in net-next,
> okay? I am currently in favor of a new netdev-feature. What do you
> think? Your RFC series could help here, too.
>
I really do not like the feature flag, it's just a bandaid over the
real problem-- in fact my goal is to eliminate NETIF_F_IP{V6}_CSUM and
just have NETIF_F_HW_CSUM. I will repost the helper patches, but we
really do need to start fixing this stuff in the drivers instead of
more hacking in the stack.

Tom

> Thanks,
> Hannes
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Hannes Frederic Sowa]

Hi Tom,

On Tue, Oct 27, 2015, at 20:19, Hannes Frederic Sowa wrote:
> On Tue, Oct 27, 2015, at 19:37, Tom Herbert wrote:
> > On Tue, Oct 27, 2015 at 11:29 AM, Hannes Frederic Sowa
> >  wrote:
> > > On Tue, Oct 27, 2015, at 18:32, Tom Herbert wrote:
> > >> On Tue, Oct 27, 2015 at 9:44 AM, Hannes Frederic Sowa
> > >>  wrote:
> > >> >
> > >> >
> > >> > On Tue, Oct 27, 2015, at 17:36, Tom Herbert wrote:> > -   
> > >> > if
> > >> > (cork->length + length > maxnonfragsize - headersize) {
> > >> >> > +   if (cork->length + length > maxnonfragsize - headersize) {
> > >> >> >  emsgsize:
> > >> >> > -   ipv6_local_error(sk, EMSGSIZE, fl6,
> > >> >> > -mtu - headersize +
> > >> >> > -sizeof(struct ipv6hdr));
> > >> >> > -   return -EMSGSIZE;
> > >> >> > -   }
> > >> >> > +   ipv6_local_error(sk, EMSGSIZE, fl6,
> > >> >> > +mtu - headersize +
> > >> >> > +sizeof(struct ipv6hdr));
> > >> >> > +   return -EMSGSIZE;
> > >> >> > }
> > >> >> >
> > >> >> > +   /* CHECKSUM_PARTIAL only with no extension headers and when
> > >> >>
> > >> >> No, please don't do this. CHECKSUM_PARTIAL should work with extension
> > >> >> headers as defined, so this is just disabling otherwise valid and
> > >> >> useful functionality. If (some) drivers have problems with this they
> > >> >> need to be identified and fixed.
> > >> >
> > >> > I don't understand. The old code already didn't allow the use of
> > >> > opt_flen with CHECKSUM_PARTIAL.
> > >> >
> > >> Then that's a problem with the old code :-). Is there any other reason
> > >> that we can't use CHECKSUM_PARTIAL with extension headers other than
> > >> lack of correct driver support?
> > >
> > > The lack of correct driver support is a big bumper, but as I wrote, I
> > > don't see a reason to not lift this restriction in net-next. I proposed
> > > a new feature flag, or by looking at your series, we could probably use
> > > the extension header okay field for that.
> > >
> > Okay, but why bother doing this for net? This problem has obviously
> > existed for a while, and even if the restriction is maintained here
> > there are still other paths that don't go through ip_append_data that
> > could trip the bug. Also, drivers are welcome to fix their issues in
> > net I believe.
> 
> I even don't know if it could be a hardware issue. Also I don't want to
> break people's communication with a patch.
> IMHO without the WARN_ON_ONCEs, which I agreed to remove, I currently
> don't see any problem for net.
> 
> You don't agree on a netdev-feature flag, indicating the driver is okay
> with hardware checksumming and extension headers? We could add this to
> net-next pretty fast, I think. It does not require people to revert this
> patch in case their driver misbehaves and we don't get a fix for it,
> soon. Also what should we do if the driver simply does not support
> extension headers + checksum offloading? Completely kill checksum
> offloading for IPv6?

I posted v3 just now. I would like to let David consider it for net
inclusion. We can work on how to lift this limitation then in net-next,
okay? I am currently in favor of a new netdev-feature. What do you
think? Your RFC series could help here, too.

Thanks,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Tom Herbert]

On Tue, Oct 27, 2015 at 5:12 PM, Hannes Frederic Sowa
 wrote:
> On Tue, Oct 27, 2015, at 23:03, Tom Herbert wrote:
>> On Tue, Oct 27, 2015 at 2:42 PM, Hannes Frederic Sowa
>>  wrote:
>> > I posted v3 just now. I would like to let David consider it for net
>> > inclusion. We can work on how to lift this limitation then in net-next,
>> > okay? I am currently in favor of a new netdev-feature. What do you
>> > think? Your RFC series could help here, too.
>> >
>> I really do not like the feature flag, it's just a bandaid over the
>> real problem-- in fact my goal is to eliminate NETIF_F_IP{V6}_CSUM and
>> just have NETIF_F_HW_CSUM. I will repost the helper patches, but we
>> really do need to start fixing this stuff in the drivers instead of
>> more hacking in the stack.
>
> It would be great if this is doable but I doubt so. There might be a lot
> of unresponsive driver maintainers and I don't see that we should simply
> eliminate IPv4 csum offloading for those drivers, too. Sometimes it is
> hard to patch drivers without documentation.
>
> I am against lifting restrictions which will have unforeseeable
> consequences for some people (as in partial communication errors) or
> having huge performance drawbacks (as in disabling ipv4 csum offloading,
> too).
>
> I could even imagine this needs to be more configurable as in how many
> extension headers some hardware can process, I fear. One extension
> header might be okay (jumping over a fragmentation header), but two... I
> simply don't know, yet. Maybe there is no problem with hardware at all.
>
Hardware that implement NETIF_F_HW_CSUM (ie. calculate csum based on
start and offset) should have no problem with extension headers. The
plea in skbuff.h for HW vendors to implement that generic algorithm
has been around a long time, but unfortunately a lot of new HW is
still do protocol specific algorithms. Realistically, I can't deploy
extension headers at scale without checksum offload anyway, so this
just degenerates into another instance where poor HW design decisions
limit the protocols and features we're able to deploy in data center.
Oh well...

> I don't really see this series as a hack. ;)
>
> Unluckily it seems we don't get feedback from the hardware about not
> being able to construct a proper checksum, so we cannot even close the
> loop and add code which warns us about misbehaving drivers.
>
> Bye,
> Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] ipv6: no CHECKSUM_PARTIAL on MSG_MORE corked sockets

[Hannes Frederic Sowa]

On Tue, Oct 27, 2015, at 23:03, Tom Herbert wrote:
> On Tue, Oct 27, 2015 at 2:42 PM, Hannes Frederic Sowa
>  wrote:
> > I posted v3 just now. I would like to let David consider it for net
> > inclusion. We can work on how to lift this limitation then in net-next,
> > okay? I am currently in favor of a new netdev-feature. What do you
> > think? Your RFC series could help here, too.
> >
> I really do not like the feature flag, it's just a bandaid over the
> real problem-- in fact my goal is to eliminate NETIF_F_IP{V6}_CSUM and
> just have NETIF_F_HW_CSUM. I will repost the helper patches, but we
> really do need to start fixing this stuff in the drivers instead of
> more hacking in the stack.

It would be great if this is doable but I doubt so. There might be a lot
of unresponsive driver maintainers and I don't see that we should simply
eliminate IPv4 csum offloading for those drivers, too. Sometimes it is
hard to patch drivers without documentation.

I am against lifting restrictions which will have unforeseeable 
consequences for some people (as in partial communication errors) or
having huge performance drawbacks (as in disabling ipv4 csum offloading,
too).

I could even imagine this needs to be more configurable as in how many
extension headers some hardware can process, I fear. One extension
header might be okay (jumping over a fragmentation header), but two... I
simply don't know, yet. Maybe there is no problem with hardware at all.

I don't really see this series as a hack. ;)

Unluckily it seems we don't get feedback from the hardware about not
being able to construct a proper checksum, so we cannot even close the
loop and add code which warns us about misbehaving drivers.

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html