| Message ID | 1458660679-7279-1-git-send-email-lrichard@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 3/22/16 9:31 AM, Lance Richardson wrote: > Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst() > before calling fib_lookup(), which means fib_table_lookup() is > using non-deterministic data at this line: > > if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { > > Fix by initializing fl4.flowi4_flags to zero. > > Signed-off-by: Lance Richardson <lrichard@redhat.com> > --- > net/ipv4/fib_frontend.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > index 21add55..896844a 100644 > --- a/net/ipv4/fib_frontend.c > +++ b/net/ipv4/fib_frontend.c > @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb) > fl4.flowi4_scope = scope; > fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0; > fl4.flowi4_tun_key.tun_id = 0; > + fl4.flowi4_flags = 0; > if (!fib_lookup(net, &fl4, &res, 0)) > return FIB_RES_PREFSRC(net, res); > } else { > Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups") I think a more robust solution is to move fl4 to this if case and init when it is declared: struct flowi4 fl4 = { .flowi4_iif = LOOPBACK_IFINDEX, .daddr = ip_hdr(skb)->saddr, .flowi4_tos = RT_TOS(ip_hdr(skb)->tos), .flowi4_scope = scope, .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0, };
----- Original Message ----- > From: "David Ahern" <dsa@cumulusnetworks.com> > To: "Lance Richardson" <lrichard@redhat.com>, netdev@vger.kernel.org > Sent: Tuesday, March 22, 2016 2:29:02 PM > Subject: Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup() > > On 3/22/16 9:31 AM, Lance Richardson wrote: > > Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst() > > before calling fib_lookup(), which means fib_table_lookup() is > > using non-deterministic data at this line: > > > > if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { > > > > Fix by initializing fl4.flowi4_flags to zero. > > > > Signed-off-by: Lance Richardson <lrichard@redhat.com> > > --- > > net/ipv4/fib_frontend.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > > index 21add55..896844a 100644 > > --- a/net/ipv4/fib_frontend.c > > +++ b/net/ipv4/fib_frontend.c > > @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb) > > fl4.flowi4_scope = scope; > > fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0; > > fl4.flowi4_tun_key.tun_id = 0; > > + fl4.flowi4_flags = 0; > > if (!fib_lookup(net, &fl4, &res, 0)) > > return FIB_RES_PREFSRC(net, res); > > } else { > > > > Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups") > > I think a more robust solution is to move fl4 to this if case and init > when it is declared: > > struct flowi4 fl4 = { > .flowi4_iif = LOOPBACK_IFINDEX, > .daddr = ip_hdr(skb)->saddr, > .flowi4_tos = RT_TOS(ip_hdr(skb)->tos), > .flowi4_scope = scope, > .flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0, > }; > Agreed... I actually debated doing something similar but opted for the smaller delta. v2 coming up. Thanks for the review, Lance
On Tue, Mar 22, 2016 at 11:29 AM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 3/22/16 9:31 AM, Lance Richardson wrote: >> >> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst() >> before calling fib_lookup(), which means fib_table_lookup() is >> using non-deterministic data at this line: >> >> if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { >> >> Fix by initializing fl4.flowi4_flags to zero. >> >> Signed-off-by: Lance Richardson <lrichard@redhat.com> >> --- >> net/ipv4/fib_frontend.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c >> index 21add55..896844a 100644 >> --- a/net/ipv4/fib_frontend.c >> +++ b/net/ipv4/fib_frontend.c >> @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb) >> fl4.flowi4_scope = scope; >> fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : >> 0; >> fl4.flowi4_tun_key.tun_id = 0; >> + fl4.flowi4_flags = 0; >> if (!fib_lookup(net, &fl4, &res, 0)) >> return FIB_RES_PREFSRC(net, res); >> } else { >> > > Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups") Why does this patch fix this commit? Even before this commit, flowi4_flags was already tested for other bit: @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const struct flowi4 *flp, nh->nh_flags & RTNH_F_LINKDOWN && !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE)) continue; - if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) { + if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { if (flp->flowi4_oif && flp->flowi4_oif != nh->nh_oif) continue; For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1.
On 3/22/16 2:45 PM, Cong Wang wrote: > @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const > struct flowi4 *flp, > nh->nh_flags & RTNH_F_LINKDOWN && > !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE)) > continue; > - if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) { > + if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { > if (flp->flowi4_oif && > flp->flowi4_oif != nh->nh_oif) > continue; > > For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1. > Arguably yes since it added the function without initializing flags. The commit I referenced (and even the VRF predecessor both of which originated in the v4.3) is the one introducing use of flow flags to the lookup. From a stable perspective going back to v4.3 is what matters.
On Tue, Mar 22, 2016 at 2:02 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 3/22/16 2:45 PM, Cong Wang wrote: >> >> @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const >> struct flowi4 *flp, >> nh->nh_flags & RTNH_F_LINKDOWN && >> !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE)) >> continue; >> - if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) { >> + if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) >> { >> if (flp->flowi4_oif && >> flp->flowi4_oif != nh->nh_oif) >> continue; >> >> For me, it looks the bug was introduce by commit >> 35ebf65e851c6d9731abc6362b1. >> > > Arguably yes since it added the function without initializing flags. > > The commit I referenced (and even the VRF predecessor both of which > originated in the v4.3) is the one introducing use of flow flags to the > lookup. From a stable perspective going back to v4.3 is what matters. > Yeah, then it is commit 613d09b30f8b589d5a9b497. It doesn't matter for backport, but matters for accuracy. ;)
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index 21add55..896844a 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb) fl4.flowi4_scope = scope; fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0; fl4.flowi4_tun_key.tun_id = 0; + fl4.flowi4_flags = 0; if (!fib_lookup(net, &fl4, &res, 0)) return FIB_RES_PREFSRC(net, res); } else {
Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst() before calling fib_lookup(), which means fib_table_lookup() is using non-deterministic data at this line: if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) { Fix by initializing fl4.flowi4_flags to zero. Signed-off-by: Lance Richardson <lrichard@redhat.com> --- net/ipv4/fib_frontend.c | 1 + 1 file changed, 1 insertion(+)