| Message ID | 20230417092900.4374-1-denis281089@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | netifd: add support of GRE tunnel ignore-df option | expand |
I have an easier patch in my private repo, maybe it's enough without a new configuration option? I'm using it in production for a gretap link which is bridged on both sides to MTU 1500 ethernet links. See https://bugzilla.kernel.org/show_bug.cgi?id=14837, which seems to say you always have to use both options: ignore-df nopmtudisc Signed-off-by: Stefan Hellermann <stefan@the2masters.de> --- a/system-linux.c +++ b/system-linux.c @@ -3496,6 +3496,7 @@ static int system_add_gre_tunnel(const c ttl = 64; nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, set_df ? 0 : 1); nla_put_u8(nlm, IFLA_GRE_TOS, tos); } Am 17.04.23 um 11:29 schrieb Denis Kalashnikov: > This is useful for GRE TAP tunnel when tunnel is added to a br-lan bridge. > In this case you need to create it with "nopmtudisc ignore-df". Otherwise > large IP-packets with DF=1 (TCP-data, large pings) will be silently dropped > (since DF=1 but stack failed to send ICMP "need fragmentation" back). But with > "ignore-df" packets with DF=1 will be fragmented. > > Signed-off-by: Denis Kalashnikov <denis281089@gmail.com> > --- > system-linux.c | 5 +++++ > system.c | 1 + > system.h | 1 + > 3 files changed, 7 insertions(+) > > diff --git a/system-linux.c b/system-linux.c > index e4041fb..4397460 100644 > --- a/system-linux.c > +++ b/system-linux.c > @@ -3500,6 +3500,11 @@ static int system_add_gre_tunnel(const char *name, const char *kind, > > nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); > > + if ((cur = tb[TUNNEL_ATTR_IGNORE_DF])) { > + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, > + blobmsg_get_bool(cur)); > + } > + > nla_put_u8(nlm, IFLA_GRE_TOS, tos); > } > > diff --git a/system.c b/system.c > index 32597c1..e773245 100644 > --- a/system.c > +++ b/system.c > @@ -21,6 +21,7 @@ static const struct blobmsg_policy tunnel_attrs[__TUNNEL_ATTR_MAX] = { > [TUNNEL_ATTR_REMOTE] = { .name = "remote", .type = BLOBMSG_TYPE_STRING }, > [TUNNEL_ATTR_MTU] = { .name = "mtu", .type = BLOBMSG_TYPE_INT32 }, > [TUNNEL_ATTR_DF] = { .name = "df", .type = BLOBMSG_TYPE_BOOL }, > + [TUNNEL_ATTR_IGNORE_DF] = { .name = "ignore-df", .type = BLOBMSG_TYPE_BOOL }, > [TUNNEL_ATTR_TTL] = { .name = "ttl", .type = BLOBMSG_TYPE_INT32 }, > [TUNNEL_ATTR_TOS] = { .name = "tos", .type = BLOBMSG_TYPE_STRING }, > [TUNNEL_ATTR_LINK] = { .name = "link", .type = BLOBMSG_TYPE_STRING }, > diff --git a/system.h b/system.h > index 1f7037d..a7a713d 100644 > --- a/system.h > +++ b/system.h > @@ -29,6 +29,7 @@ enum tunnel_param { > TUNNEL_ATTR_LOCAL, > TUNNEL_ATTR_MTU, > TUNNEL_ATTR_DF, > + TUNNEL_ATTR_IGNORE_DF, > TUNNEL_ATTR_TTL, > TUNNEL_ATTR_TOS, > TUNNEL_ATTR_LINK,
I'm not sure you want it to be unconditional. When I wrote the option for netlink and iproute2, it was for some very specialized scenarios. > On Apr 24, 2023, at 3:14 AM, Stefan Hellermann <stefan@the2masters.de> wrote: > > I have an easier patch in my private repo, maybe it's enough without a new configuration option? I'm using it in production for a gretap link which is bridged on both sides to MTU 1500 ethernet links. > > See https://bugzilla.kernel.org/show_bug.cgi?id=14837, which seems to say you always have to use both options: ignore-df nopmtudisc > > Signed-off-by: Stefan Hellermann <stefan@the2masters.de> > > --- a/system-linux.c > +++ b/system-linux.c > @@ -3496,6 +3496,7 @@ static int system_add_gre_tunnel(const c > ttl = 64; > > nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); > + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, set_df ? 0 : 1); > > nla_put_u8(nlm, IFLA_GRE_TOS, tos); > } > > Am 17.04.23 um 11:29 schrieb Denis Kalashnikov: >> This is useful for GRE TAP tunnel when tunnel is added to a br-lan bridge. >> In this case you need to create it with "nopmtudisc ignore-df". Otherwise >> large IP-packets with DF=1 (TCP-data, large pings) will be silently dropped >> (since DF=1 but stack failed to send ICMP "need fragmentation" back). But with >> "ignore-df" packets with DF=1 will be fragmented. >> >> Signed-off-by: Denis Kalashnikov <denis281089@gmail.com> >> --- >> system-linux.c | 5 +++++ >> system.c | 1 + >> system.h | 1 + >> 3 files changed, 7 insertions(+) >> >> diff --git a/system-linux.c b/system-linux.c >> index e4041fb..4397460 100644 >> --- a/system-linux.c >> +++ b/system-linux.c >> @@ -3500,6 +3500,11 @@ static int system_add_gre_tunnel(const char *name, const char *kind, >> nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); >> + if ((cur = tb[TUNNEL_ATTR_IGNORE_DF])) { >> + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, >> + blobmsg_get_bool(cur)); >> + } >> + >> nla_put_u8(nlm, IFLA_GRE_TOS, tos); >> } >> diff --git a/system.c b/system.c >> index 32597c1..e773245 100644 >> --- a/system.c >> +++ b/system.c >> @@ -21,6 +21,7 @@ static const struct blobmsg_policy tunnel_attrs[__TUNNEL_ATTR_MAX] = { >> [TUNNEL_ATTR_REMOTE] = { .name = "remote", .type = BLOBMSG_TYPE_STRING }, >> [TUNNEL_ATTR_MTU] = { .name = "mtu", .type = BLOBMSG_TYPE_INT32 }, >> [TUNNEL_ATTR_DF] = { .name = "df", .type = BLOBMSG_TYPE_BOOL }, >> + [TUNNEL_ATTR_IGNORE_DF] = { .name = "ignore-df", .type = BLOBMSG_TYPE_BOOL }, >> [TUNNEL_ATTR_TTL] = { .name = "ttl", .type = BLOBMSG_TYPE_INT32 }, >> [TUNNEL_ATTR_TOS] = { .name = "tos", .type = BLOBMSG_TYPE_STRING }, >> [TUNNEL_ATTR_LINK] = { .name = "link", .type = BLOBMSG_TYPE_STRING }, >> diff --git a/system.h b/system.h >> index 1f7037d..a7a713d 100644 >> --- a/system.h >> +++ b/system.h >> @@ -29,6 +29,7 @@ enum tunnel_param { >> TUNNEL_ATTR_LOCAL, >> TUNNEL_ATTR_MTU, >> TUNNEL_ATTR_DF, >> + TUNNEL_ATTR_IGNORE_DF, >> TUNNEL_ATTR_TTL, >> TUNNEL_ATTR_TOS, >> TUNNEL_ATTR_LINK, > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
It seems that we have two options: all is good with the tunnel path, so in this case we use default "pmtudisc noignore-df". Or something wrong and ICMP replies with "fragmentation needed" do not come back, so in this case we use "nopmtudisc ignore-df". But I would leave the possibility of selective control of enabling of these options, just in case. So, I agree with Philip. пн, 24 апр. 2023 г. в 17:33, Philip Prindeville <philipp_subx@redfish-solutions.com>: > > I'm not sure you want it to be unconditional. > > When I wrote the option for netlink and iproute2, it was for some very specialized scenarios. > > > > On Apr 24, 2023, at 3:14 AM, Stefan Hellermann <stefan@the2masters.de> wrote: > > > > I have an easier patch in my private repo, maybe it's enough without a new configuration option? I'm using it in production for a gretap link which is bridged on both sides to MTU 1500 ethernet links. > > > > See https://bugzilla.kernel.org/show_bug.cgi?id=14837, which seems to say you always have to use both options: ignore-df nopmtudisc > > > > Signed-off-by: Stefan Hellermann <stefan@the2masters.de> > > > > --- a/system-linux.c > > +++ b/system-linux.c > > @@ -3496,6 +3496,7 @@ static int system_add_gre_tunnel(const c > > ttl = 64; > > > > nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); > > + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, set_df ? 0 : 1); > > > > nla_put_u8(nlm, IFLA_GRE_TOS, tos); > > } > > > > Am 17.04.23 um 11:29 schrieb Denis Kalashnikov: > >> This is useful for GRE TAP tunnel when tunnel is added to a br-lan bridge. > >> In this case you need to create it with "nopmtudisc ignore-df". Otherwise > >> large IP-packets with DF=1 (TCP-data, large pings) will be silently dropped > >> (since DF=1 but stack failed to send ICMP "need fragmentation" back). But with > >> "ignore-df" packets with DF=1 will be fragmented. > >> > >> Signed-off-by: Denis Kalashnikov <denis281089@gmail.com> > >> --- > >> system-linux.c | 5 +++++ > >> system.c | 1 + > >> system.h | 1 + > >> 3 files changed, 7 insertions(+) > >> > >> diff --git a/system-linux.c b/system-linux.c > >> index e4041fb..4397460 100644 > >> --- a/system-linux.c > >> +++ b/system-linux.c > >> @@ -3500,6 +3500,11 @@ static int system_add_gre_tunnel(const char *name, const char *kind, > >> nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); > >> + if ((cur = tb[TUNNEL_ATTR_IGNORE_DF])) { > >> + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, > >> + blobmsg_get_bool(cur)); > >> + } > >> + > >> nla_put_u8(nlm, IFLA_GRE_TOS, tos); > >> } > >> diff --git a/system.c b/system.c > >> index 32597c1..e773245 100644 > >> --- a/system.c > >> +++ b/system.c > >> @@ -21,6 +21,7 @@ static const struct blobmsg_policy tunnel_attrs[__TUNNEL_ATTR_MAX] = { > >> [TUNNEL_ATTR_REMOTE] = { .name = "remote", .type = BLOBMSG_TYPE_STRING }, > >> [TUNNEL_ATTR_MTU] = { .name = "mtu", .type = BLOBMSG_TYPE_INT32 }, > >> [TUNNEL_ATTR_DF] = { .name = "df", .type = BLOBMSG_TYPE_BOOL }, > >> + [TUNNEL_ATTR_IGNORE_DF] = { .name = "ignore-df", .type = BLOBMSG_TYPE_BOOL }, > >> [TUNNEL_ATTR_TTL] = { .name = "ttl", .type = BLOBMSG_TYPE_INT32 }, > >> [TUNNEL_ATTR_TOS] = { .name = "tos", .type = BLOBMSG_TYPE_STRING }, > >> [TUNNEL_ATTR_LINK] = { .name = "link", .type = BLOBMSG_TYPE_STRING }, > >> diff --git a/system.h b/system.h > >> index 1f7037d..a7a713d 100644 > >> --- a/system.h > >> +++ b/system.h > >> @@ -29,6 +29,7 @@ enum tunnel_param { > >> TUNNEL_ATTR_LOCAL, > >> TUNNEL_ATTR_MTU, > >> TUNNEL_ATTR_DF, > >> + TUNNEL_ATTR_IGNORE_DF, > >> TUNNEL_ATTR_TTL, > >> TUNNEL_ATTR_TOS, > >> TUNNEL_ATTR_LINK, > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel >
diff --git a/system-linux.c b/system-linux.c index e4041fb..4397460 100644 --- a/system-linux.c +++ b/system-linux.c @@ -3500,6 +3500,11 @@ static int system_add_gre_tunnel(const char *name, const char *kind, nla_put_u8(nlm, IFLA_GRE_PMTUDISC, set_df ? 1 : 0); + if ((cur = tb[TUNNEL_ATTR_IGNORE_DF])) { + nla_put_u8(nlm, IFLA_GRE_IGNORE_DF, + blobmsg_get_bool(cur)); + } + nla_put_u8(nlm, IFLA_GRE_TOS, tos); } diff --git a/system.c b/system.c index 32597c1..e773245 100644 --- a/system.c +++ b/system.c @@ -21,6 +21,7 @@ static const struct blobmsg_policy tunnel_attrs[__TUNNEL_ATTR_MAX] = { [TUNNEL_ATTR_REMOTE] = { .name = "remote", .type = BLOBMSG_TYPE_STRING }, [TUNNEL_ATTR_MTU] = { .name = "mtu", .type = BLOBMSG_TYPE_INT32 }, [TUNNEL_ATTR_DF] = { .name = "df", .type = BLOBMSG_TYPE_BOOL }, + [TUNNEL_ATTR_IGNORE_DF] = { .name = "ignore-df", .type = BLOBMSG_TYPE_BOOL }, [TUNNEL_ATTR_TTL] = { .name = "ttl", .type = BLOBMSG_TYPE_INT32 }, [TUNNEL_ATTR_TOS] = { .name = "tos", .type = BLOBMSG_TYPE_STRING }, [TUNNEL_ATTR_LINK] = { .name = "link", .type = BLOBMSG_TYPE_STRING }, diff --git a/system.h b/system.h index 1f7037d..a7a713d 100644 --- a/system.h +++ b/system.h @@ -29,6 +29,7 @@ enum tunnel_param { TUNNEL_ATTR_LOCAL, TUNNEL_ATTR_MTU, TUNNEL_ATTR_DF, + TUNNEL_ATTR_IGNORE_DF, TUNNEL_ATTR_TTL, TUNNEL_ATTR_TOS, TUNNEL_ATTR_LINK,
This is useful for GRE TAP tunnel when tunnel is added to a br-lan bridge. In this case you need to create it with "nopmtudisc ignore-df". Otherwise large IP-packets with DF=1 (TCP-data, large pings) will be silently dropped (since DF=1 but stack failed to send ICMP "need fragmentation" back). But with "ignore-df" packets with DF=1 will be fragmented. Signed-off-by: Denis Kalashnikov <denis281089@gmail.com> --- system-linux.c | 5 +++++ system.c | 1 + system.h | 1 + 3 files changed, 7 insertions(+)