| Message ID | 20160902000155.12906-1-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Sep 1, 2016 at 5:01 PM, Joe Stringer <joe@ovn.org> wrote: > The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() > call, which does deeper (eg l4proto) validation. It was previously > thought that using the NF_INET_ROUTING hook for this function on older > kernels would trigger kernel panics due to a dependency on the > unpopulated skb->dev, however during recent testing on a variety of > platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest > distribution kernels and the OVS kernel module testsuite, no such kernel > panics were observed. Therefore it appears to be safe to bring this in > line with upstream without any other workarounds. > > Reported-by: Jesse Gross <jesse@kernel.org> > Signed-off-by: Joe Stringer <joe@ovn.org> If you are confident that it doesn't cause problems on older kernels, the change looks obviously correct to me relative to upstream. Acked-by: Jesse Gross <jesse@kernel.org>
On 1 September 2016 at 18:08, Jesse Gross <jesse@kernel.org> wrote: > On Thu, Sep 1, 2016 at 5:01 PM, Joe Stringer <joe@ovn.org> wrote: >> The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() >> call, which does deeper (eg l4proto) validation. It was previously >> thought that using the NF_INET_ROUTING hook for this function on older >> kernels would trigger kernel panics due to a dependency on the >> unpopulated skb->dev, however during recent testing on a variety of >> platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest >> distribution kernels and the OVS kernel module testsuite, no such kernel >> panics were observed. Therefore it appears to be safe to bring this in >> line with upstream without any other workarounds. >> >> Reported-by: Jesse Gross <jesse@kernel.org> >> Signed-off-by: Joe Stringer <joe@ovn.org> > > If you are confident that it doesn't cause problems on older kernels, > the change looks obviously correct to me relative to upstream. Unfortunately I don't have concrete details of the original issue, so I can't say this with strong confidence. I don't think it was ever a problem upstream, (ie 4.3+), so we /could/ keep it as NF_INET_FORWARD on kernels older than that..
On Wed, Sep 7, 2016 at 5:18 PM, Joe Stringer <joe@ovn.org> wrote: > On 1 September 2016 at 18:08, Jesse Gross <jesse@kernel.org> wrote: >> On Thu, Sep 1, 2016 at 5:01 PM, Joe Stringer <joe@ovn.org> wrote: >>> The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() >>> call, which does deeper (eg l4proto) validation. It was previously >>> thought that using the NF_INET_ROUTING hook for this function on older >>> kernels would trigger kernel panics due to a dependency on the >>> unpopulated skb->dev, however during recent testing on a variety of >>> platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest >>> distribution kernels and the OVS kernel module testsuite, no such kernel >>> panics were observed. Therefore it appears to be safe to bring this in >>> line with upstream without any other workarounds. >>> >>> Reported-by: Jesse Gross <jesse@kernel.org> >>> Signed-off-by: Joe Stringer <joe@ovn.org> >> >> If you are confident that it doesn't cause problems on older kernels, >> the change looks obviously correct to me relative to upstream. > > Unfortunately I don't have concrete details of the original issue, so I > can't say this with strong confidence. > > I don't think it was ever a problem upstream, (ie 4.3+), so we /could/ > keep it as NF_INET_FORWARD on kernels older than that.. I think if you've tested it on the major distribution kernels then it's unlikely we'll see problems in practice. It's probably not worth retaining the old symbol based on an issue that we don't even know the details of, so I would just go ahead with this patch.
On 8 September 2016 at 08:49, Jesse Gross <jesse@kernel.org> wrote: > On Wed, Sep 7, 2016 at 5:18 PM, Joe Stringer <joe@ovn.org> wrote: >> On 1 September 2016 at 18:08, Jesse Gross <jesse@kernel.org> wrote: >>> On Thu, Sep 1, 2016 at 5:01 PM, Joe Stringer <joe@ovn.org> wrote: >>>> The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() >>>> call, which does deeper (eg l4proto) validation. It was previously >>>> thought that using the NF_INET_ROUTING hook for this function on older >>>> kernels would trigger kernel panics due to a dependency on the >>>> unpopulated skb->dev, however during recent testing on a variety of >>>> platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest >>>> distribution kernels and the OVS kernel module testsuite, no such kernel >>>> panics were observed. Therefore it appears to be safe to bring this in >>>> line with upstream without any other workarounds. >>>> >>>> Reported-by: Jesse Gross <jesse@kernel.org> >>>> Signed-off-by: Joe Stringer <joe@ovn.org> >>> >>> If you are confident that it doesn't cause problems on older kernels, >>> the change looks obviously correct to me relative to upstream. >> >> Unfortunately I don't have concrete details of the original issue, so I >> can't say this with strong confidence. >> >> I don't think it was ever a problem upstream, (ie 4.3+), so we /could/ >> keep it as NF_INET_FORWARD on kernels older than that.. > > I think if you've tested it on the major distribution kernels then > it's unlikely we'll see problems in practice. It's probably not worth > retaining the old symbol based on an issue that we don't even know the > details of, so I would just go ahead with this patch. That sounds reasonable. If down the line we rediscover this issue, we can consider whether to add back a workaround for it. Applied to master. Any opinions on backporting to branch-2.[56]?
On Fri, Sep 9, 2016 at 2:37 PM, Joe Stringer <joe@ovn.org> wrote: > On 8 September 2016 at 08:49, Jesse Gross <jesse@kernel.org> wrote: >> On Wed, Sep 7, 2016 at 5:18 PM, Joe Stringer <joe@ovn.org> wrote: >>> On 1 September 2016 at 18:08, Jesse Gross <jesse@kernel.org> wrote: >>>> On Thu, Sep 1, 2016 at 5:01 PM, Joe Stringer <joe@ovn.org> wrote: >>>>> The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() >>>>> call, which does deeper (eg l4proto) validation. It was previously >>>>> thought that using the NF_INET_ROUTING hook for this function on older >>>>> kernels would trigger kernel panics due to a dependency on the >>>>> unpopulated skb->dev, however during recent testing on a variety of >>>>> platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest >>>>> distribution kernels and the OVS kernel module testsuite, no such kernel >>>>> panics were observed. Therefore it appears to be safe to bring this in >>>>> line with upstream without any other workarounds. >>>>> >>>>> Reported-by: Jesse Gross <jesse@kernel.org> >>>>> Signed-off-by: Joe Stringer <joe@ovn.org> >>>> >>>> If you are confident that it doesn't cause problems on older kernels, >>>> the change looks obviously correct to me relative to upstream. >>> >>> Unfortunately I don't have concrete details of the original issue, so I >>> can't say this with strong confidence. >>> >>> I don't think it was ever a problem upstream, (ie 4.3+), so we /could/ >>> keep it as NF_INET_FORWARD on kernels older than that.. >> >> I think if you've tested it on the major distribution kernels then >> it's unlikely we'll see problems in practice. It's probably not worth >> retaining the old symbol based on an issue that we don't even know the >> details of, so I would just go ahead with this patch. > > That sounds reasonable. If down the line we rediscover this issue, we > can consider whether to add back a workaround for it. > > Applied to master. > > Any opinions on backporting to branch-2.[56]? I would be more inclined to err on the side of caution for the release branches. This doesn't fix any major bugs - I know this does change behavior a bit but in my mind the most important part of the change is to make things consistent with upstream. As a result, I would probably keep this on master only.
diff --git a/datapath/conntrack.c b/datapath/conntrack.c index ddfb0c42b379..a2fc450edc05 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -772,7 +772,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key, /* Repeat if requested, see nf_iterate(). */ do { err = nf_conntrack_in(net, info->family, - NF_INET_FORWARD, skb); + NF_INET_PRE_ROUTING, skb); } while (err == NF_REPEAT); if (err != NF_ACCEPT)
The upstream code uses NF_INET_PRE_ROUTING hook for the nf_conntrack_in() call, which does deeper (eg l4proto) validation. It was previously thought that using the NF_INET_ROUTING hook for this function on older kernels would trigger kernel panics due to a dependency on the unpopulated skb->dev, however during recent testing on a variety of platforms (Centos7.[12], Ubuntu 1[46].04, Fedora23) using the latest distribution kernels and the OVS kernel module testsuite, no such kernel panics were observed. Therefore it appears to be safe to bring this in line with upstream without any other workarounds. Reported-by: Jesse Gross <jesse@kernel.org> Signed-off-by: Joe Stringer <joe@ovn.org> --- datapath/conntrack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)