| Message ID | 02bef0921778d2053ab63140c31704712bb5a864.1611825446.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62fafcd63139920eb25b3fbf154177ce3e6f3232 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add support for ip generic checksum offload for gre | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: andriin@fb.com daniel@iogearbox.net bjorn@kernel.org edumazet@google.com ap420073@gmail.com ast@kernel.org xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@gmail.com> wrote: > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > for HW, which includes not only for TCP/UDP csum, but also for other > protocols' csum like GRE's. > > However, in skb_csum_hwoffload_help() it only checks features against > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > to do csum. > > This patch is to support ip generic csum processing by checking > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > Note that we're using skb->csum_offset to check if it's a TCP/UDP > proctol, this might be fragile. However, as Alex said, for now we > only have a few L4 protocols that are requesting Tx csum offload, > we'd better fix this until a new protocol comes with a same csum > offset. > > v1->v2: > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > if it's an UDP/TCP csum packet. > v2->v3: > - add a note in the changelog, as Willem suggested. > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/core/dev.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 6df3f1b..aae116d 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > skb_crc32c_csum_help(skb); > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > + if (features & NETIF_F_HW_CSUM) > + return 0; > + > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { Should this check the specific feature flag against skb->protocol? I don't know if there are actually instances that only support one of the two flags. > + switch (skb->csum_offset) { > + case offsetof(struct tcphdr, check): > + case offsetof(struct udphdr, check): > + return 0; > + } > + } > + > + return skb_checksum_help(skb); > } > EXPORT_SYMBOL(skb_csum_hwoffload_help); > > -- > 2.1.0 >
On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > > for HW, which includes not only for TCP/UDP csum, but also for other > > protocols' csum like GRE's. > > > > However, in skb_csum_hwoffload_help() it only checks features against > > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > > to do csum. > > > > This patch is to support ip generic csum processing by checking > > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > > > Note that we're using skb->csum_offset to check if it's a TCP/UDP > > proctol, this might be fragile. However, as Alex said, for now we > > only have a few L4 protocols that are requesting Tx csum offload, > > we'd better fix this until a new protocol comes with a same csum > > offset. > > > > v1->v2: > > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > > if it's an UDP/TCP csum packet. > > v2->v3: > > - add a note in the changelog, as Willem suggested. > > > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/core/dev.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 6df3f1b..aae116d 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > skb_crc32c_csum_help(skb); > > > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > > + if (features & NETIF_F_HW_CSUM) > > + return 0; > > + > > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > > Should this check the specific feature flag against skb->protocol? I > don't know if there are actually instances that only support one of > the two flags. The issue is at a certain point we start excluding devices that were previously working. All this patch is really doing is using the checksum offset to identify the cases that were previously UDP or TCP offloads and letting those through with the legacy path, while any offsets that are not those two, such as the GRE checksum will now have to be explicitly caught by the NETIF_F_HW_CSUM case and not accepted by the other cases.
On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > > > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > > > for HW, which includes not only for TCP/UDP csum, but also for other > > > protocols' csum like GRE's. > > > > > > However, in skb_csum_hwoffload_help() it only checks features against > > > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > > > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > > > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > > > to do csum. > > > > > > This patch is to support ip generic csum processing by checking > > > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > > > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > > > > > Note that we're using skb->csum_offset to check if it's a TCP/UDP > > > proctol, this might be fragile. However, as Alex said, for now we > > > only have a few L4 protocols that are requesting Tx csum offload, > > > we'd better fix this until a new protocol comes with a same csum > > > offset. > > > > > > v1->v2: > > > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > > > if it's an UDP/TCP csum packet. > > > v2->v3: > > > - add a note in the changelog, as Willem suggested. > > > > > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > net/core/dev.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > index 6df3f1b..aae116d 100644 > > > --- a/net/core/dev.c > > > +++ b/net/core/dev.c > > > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > > > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > > skb_crc32c_csum_help(skb); > > > > > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > > > + if (features & NETIF_F_HW_CSUM) > > > + return 0; > > > + > > > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > > > > Should this check the specific feature flag against skb->protocol? I > > don't know if there are actually instances that only support one of > > the two flags. > > The issue is at a certain point we start excluding devices that were > previously working. > > All this patch is really doing is using the checksum offset to > identify the cases that were previously UDP or TCP offloads and > letting those through with the legacy path, while any offsets that are > not those two, such as the GRE checksum will now have to be explicitly > caught by the NETIF_F_HW_CSUM case and not accepted by the other > cases. I understand. But letting through an IPv6 packet to a nic that advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still incorrect, right?
On Thu, Jan 28, 2021 at 12:00 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > > > > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > > > > for HW, which includes not only for TCP/UDP csum, but also for other > > > > protocols' csum like GRE's. > > > > > > > > However, in skb_csum_hwoffload_help() it only checks features against > > > > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > > > > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > > > > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > > > > to do csum. > > > > > > > > This patch is to support ip generic csum processing by checking > > > > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > > > > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > > > > > > > Note that we're using skb->csum_offset to check if it's a TCP/UDP > > > > proctol, this might be fragile. However, as Alex said, for now we > > > > only have a few L4 protocols that are requesting Tx csum offload, > > > > we'd better fix this until a new protocol comes with a same csum > > > > offset. > > > > > > > > v1->v2: > > > > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > > > > if it's an UDP/TCP csum packet. > > > > v2->v3: > > > > - add a note in the changelog, as Willem suggested. > > > > > > > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > --- > > > > net/core/dev.c | 13 ++++++++++++- > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > > index 6df3f1b..aae116d 100644 > > > > --- a/net/core/dev.c > > > > +++ b/net/core/dev.c > > > > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > > > > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > > > skb_crc32c_csum_help(skb); > > > > > > > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > > > > + if (features & NETIF_F_HW_CSUM) > > > > + return 0; > > > > + > > > > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > > > > > > Should this check the specific feature flag against skb->protocol? I > > > don't know if there are actually instances that only support one of > > > the two flags. > > > > The issue is at a certain point we start excluding devices that were > > previously working. > > > > All this patch is really doing is using the checksum offset to > > identify the cases that were previously UDP or TCP offloads and > > letting those through with the legacy path, while any offsets that are > > not those two, such as the GRE checksum will now have to be explicitly > > caught by the NETIF_F_HW_CSUM case and not accepted by the other > > cases. > > I understand. But letting through an IPv6 packet to a nic that > advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still > incorrect, right? That all depends. The problem is if we are going to look at protocol we essentially have to work our way through a number of fields and sort out if there are tunnels or not and if so what the protocol for the inner headers are and if that is supported. It might make more sense in that case to look at incorporating a v4/v6 specific check into netif_skb_features so we could mask off the bit there. The question i would have is how has this code been working up until now without that check? If we are broken outright and need to add it then maybe this should be deemed more of a fix and pushed for net with the added protocol bit masking added.
On Thu, Jan 28, 2021 at 4:42 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Thu, Jan 28, 2021 at 12:00 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck > > <alexander.duyck@gmail.com> wrote: > > > > > > On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > > > On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > > > > > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > > > > > for HW, which includes not only for TCP/UDP csum, but also for other > > > > > protocols' csum like GRE's. > > > > > > > > > > However, in skb_csum_hwoffload_help() it only checks features against > > > > > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > > > > > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > > > > > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > > > > > to do csum. > > > > > > > > > > This patch is to support ip generic csum processing by checking > > > > > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > > > > > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > > > > > > > > > Note that we're using skb->csum_offset to check if it's a TCP/UDP > > > > > proctol, this might be fragile. However, as Alex said, for now we > > > > > only have a few L4 protocols that are requesting Tx csum offload, > > > > > we'd better fix this until a new protocol comes with a same csum > > > > > offset. > > > > > > > > > > v1->v2: > > > > > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > > > > > if it's an UDP/TCP csum packet. > > > > > v2->v3: > > > > > - add a note in the changelog, as Willem suggested. > > > > > > > > > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > --- > > > > > net/core/dev.c | 13 ++++++++++++- > > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > > > index 6df3f1b..aae116d 100644 > > > > > --- a/net/core/dev.c > > > > > +++ b/net/core/dev.c > > > > > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > > > > > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > > > > skb_crc32c_csum_help(skb); > > > > > > > > > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > > > > > + if (features & NETIF_F_HW_CSUM) > > > > > + return 0; > > > > > + > > > > > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > > > > > > > > Should this check the specific feature flag against skb->protocol? I > > > > don't know if there are actually instances that only support one of > > > > the two flags. > > > > > > The issue is at a certain point we start excluding devices that were > > > previously working. > > > > > > All this patch is really doing is using the checksum offset to > > > identify the cases that were previously UDP or TCP offloads and > > > letting those through with the legacy path, while any offsets that are > > > not those two, such as the GRE checksum will now have to be explicitly > > > caught by the NETIF_F_HW_CSUM case and not accepted by the other > > > cases. > > > > I understand. But letting through an IPv6 packet to a nic that > > advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still > > incorrect, right? > > That all depends. The problem is if we are going to look at protocol > we essentially have to work our way through a number of fields and > sort out if there are tunnels or not and if so what the protocol for > the inner headers are and if that is supported. It might make more > sense in that case to look at incorporating a v4/v6 specific check > into netif_skb_features so we could mask off the bit there. > > The question i would have is how has this code been working up until > now without that check? If we are broken outright and need to add it > then maybe this should be deemed more of a fix and pushed for net with > the added protocol bit masking added. Fair enough. I agree that this patch does not make anything worse, as it actually tightens the rules.
diff --git a/net/core/dev.c b/net/core/dev.c index 6df3f1b..aae116d 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb); - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); + if (features & NETIF_F_HW_CSUM) + return 0; + + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { + switch (skb->csum_offset) { + case offsetof(struct tcphdr, check): + case offsetof(struct udphdr, check): + return 0; + } + } + + return skb_checksum_help(skb); } EXPORT_SYMBOL(skb_csum_hwoffload_help);
NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload for HW, which includes not only for TCP/UDP csum, but also for other protocols' csum like GRE's. However, in skb_csum_hwoffload_help() it only checks features against NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP packet and the features doesn't support NETIF_F_HW_CSUM, but supports NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW to do csum. This patch is to support ip generic csum processing by checking NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) only for TCP and UDP. Note that we're using skb->csum_offset to check if it's a TCP/UDP proctol, this might be fragile. However, as Alex said, for now we only have a few L4 protocols that are requesting Tx csum offload, we'd better fix this until a new protocol comes with a same csum offset. v1->v2: - not extend skb->csum_not_inet, but use skb->csum_offset to tell if it's an UDP/TCP csum packet. v2->v3: - add a note in the changelog, as Willem suggested. Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/core/dev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)