* re: ipvs: Pull out crosses_local_route_boundary logic
@ 2014-10-01 17:45 Dan Carpenter
2014-10-01 19:37 ` Julian Anastasov
2014-10-06 0:54 ` Alex Gartrell
0 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2014-10-01 17:45 UTC (permalink / raw)
To: agartrell; +Cc: lvs-devel
Hello Alex Gartrell,
This is a semi-automatic email about new static checker warnings.
The patch 4a4739d56b00: "ipvs: Pull out crosses_local_route_boundary
logic" from Sep 9, 2014, leads to the following Smatch complaint:
net/netfilter/ipvs/ip_vs_xmit.c:318 __ip_vs_get_out_rt()
error: we previously assumed 'dest' could be null (see line 312)
net/netfilter/ipvs/ip_vs_xmit.c
270 if (dest) {
^^^^
Old check.
271 dest_dst = __ip_vs_dst_check(dest);
272 if (likely(dest_dst))
273 rt = (struct rtable *) dest_dst->dst_cache;
274 else {
275 dest_dst = ip_vs_dest_dst_alloc();
276 spin_lock_bh(&dest->dst_lock);
277 if (!dest_dst) {
278 __ip_vs_dst_set(dest, NULL, NULL, 0);
279 spin_unlock_bh(&dest->dst_lock);
280 goto err_unreach;
281 }
282 rt = do_output_route4(net, dest->addr.ip, rt_mode,
283 &dest_dst->dst_saddr.ip);
284 if (!rt) {
285 __ip_vs_dst_set(dest, NULL, NULL, 0);
286 spin_unlock_bh(&dest->dst_lock);
287 ip_vs_dest_dst_free(dest_dst);
288 goto err_unreach;
289 }
290 __ip_vs_dst_set(dest, dest_dst, &rt->dst, 0);
291 spin_unlock_bh(&dest->dst_lock);
292 IP_VS_DBG(10, "new dst %pI4, src %pI4, refcnt=%d\n",
293 &dest->addr.ip, &dest_dst->dst_saddr.ip,
294 atomic_read(&rt->dst.__refcnt));
295 }
296 daddr = dest->addr.ip;
297 if (ret_saddr)
298 *ret_saddr = dest_dst->dst_saddr.ip;
299 } else {
300 __be32 saddr = htonl(INADDR_ANY);
301
302 noref = 0;
303
304 /* For such unconfigured boxes avoid many route lookups
305 * for performance reasons because we do not remember saddr
306 */
307 rt_mode &= ~IP_VS_RT_MODE_CONNECT;
308 rt = do_output_route4(net, daddr, rt_mode, &saddr);
309 if (!rt)
310 goto err_unreach;
311 if (ret_saddr)
312 *ret_saddr = saddr;
313 }
314
315 local = (rt->rt_flags & RTCF_LOCAL) ? 1 : 0;
316 if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
317 local))) {
318 IP_VS_DBG_RL("We are crossing local and non-local addresses"
319 " daddr=%pI4\n", &dest->addr.ip);
^^^^^^^^^^^^^
New unchecked dereference.
320 goto err_put;
Also:
net/netfilter/ipvs/ip_vs_xmit.c:460 __ip_vs_get_out_rt_v6() error: we previously assumed 'dest' could be null (see line 415)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* re: ipvs: Pull out crosses_local_route_boundary logic
2014-10-01 17:45 ipvs: Pull out crosses_local_route_boundary logic Dan Carpenter
@ 2014-10-01 19:37 ` Julian Anastasov
2014-10-05 23:47 ` Alex Gartrell
2014-10-06 0:54 ` Alex Gartrell
1 sibling, 1 reply; 15+ messages in thread
From: Julian Anastasov @ 2014-10-01 19:37 UTC (permalink / raw)
To: Dan Carpenter; +Cc: agartrell, lvs-devel
Hello,
On Wed, 1 Oct 2014, Dan Carpenter wrote:
> Hello Alex Gartrell,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4a4739d56b00: "ipvs: Pull out crosses_local_route_boundary
> logic" from Sep 9, 2014, leads to the following Smatch complaint:
>
> net/netfilter/ipvs/ip_vs_xmit.c:318 __ip_vs_get_out_rt()
> error: we previously assumed 'dest' could be null (see line 312)
>
> net/netfilter/ipvs/ip_vs_xmit.c
> 270 if (dest) {
> ^^^^
> Old check.
>
> 271 dest_dst = __ip_vs_dst_check(dest);
> 272 if (likely(dest_dst))
> 273 rt = (struct rtable *) dest_dst->dst_cache;
> 274 else {
> 275 dest_dst = ip_vs_dest_dst_alloc();
> 276 spin_lock_bh(&dest->dst_lock);
> 277 if (!dest_dst) {
> 278 __ip_vs_dst_set(dest, NULL, NULL, 0);
> 279 spin_unlock_bh(&dest->dst_lock);
> 280 goto err_unreach;
> 281 }
> 282 rt = do_output_route4(net, dest->addr.ip, rt_mode,
> 283 &dest_dst->dst_saddr.ip);
> 284 if (!rt) {
> 285 __ip_vs_dst_set(dest, NULL, NULL, 0);
> 286 spin_unlock_bh(&dest->dst_lock);
> 287 ip_vs_dest_dst_free(dest_dst);
> 288 goto err_unreach;
> 289 }
> 290 __ip_vs_dst_set(dest, dest_dst, &rt->dst, 0);
> 291 spin_unlock_bh(&dest->dst_lock);
> 292 IP_VS_DBG(10, "new dst %pI4, src %pI4, refcnt=%d\n",
> 293 &dest->addr.ip, &dest_dst->dst_saddr.ip,
> 294 atomic_read(&rt->dst.__refcnt));
> 295 }
> 296 daddr = dest->addr.ip;
> 297 if (ret_saddr)
> 298 *ret_saddr = dest_dst->dst_saddr.ip;
> 299 } else {
> 300 __be32 saddr = htonl(INADDR_ANY);
> 301
> 302 noref = 0;
> 303
> 304 /* For such unconfigured boxes avoid many route lookups
> 305 * for performance reasons because we do not remember saddr
> 306 */
> 307 rt_mode &= ~IP_VS_RT_MODE_CONNECT;
> 308 rt = do_output_route4(net, daddr, rt_mode, &saddr);
> 309 if (!rt)
> 310 goto err_unreach;
> 311 if (ret_saddr)
> 312 *ret_saddr = saddr;
> 313 }
> 314
> 315 local = (rt->rt_flags & RTCF_LOCAL) ? 1 : 0;
> 316 if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> 317 local))) {
> 318 IP_VS_DBG_RL("We are crossing local and non-local addresses"
> 319 " daddr=%pI4\n", &dest->addr.ip);
> ^^^^^^^^^^^^^
Good catch. In fact, I requested Alex to print
daddr in this case but then we both missed this dest here...
Alex, both v4 (&daddr) and v6 (daddr) should be fixed.
> New unchecked dereference.
>
> 320 goto err_put;
>
> Also:
> net/netfilter/ipvs/ip_vs_xmit.c:460 __ip_vs_get_out_rt_v6() error: we previously assumed 'dest' could be null (see line 415)
>
> regards,
> dan carpenter
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ipvs: Pull out crosses_local_route_boundary logic
2014-10-01 19:37 ` Julian Anastasov
@ 2014-10-05 23:47 ` Alex Gartrell
2014-10-06 7:32 ` Simon Horman
0 siblings, 1 reply; 15+ messages in thread
From: Alex Gartrell @ 2014-10-05 23:47 UTC (permalink / raw)
To: Julian Anastasov, Dan Carpenter; +Cc: lvs-devel
Hey everyone,
On 10/1/14 12:37 PM, Julian Anastasov wrote:
>> 312 *ret_saddr = saddr;
>> 313 }
>> 314
>> 315 local = (rt->rt_flags & RTCF_LOCAL) ? 1 : 0;
>> 316 if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
>> 317 local))) {
>> 318 IP_VS_DBG_RL("We are crossing local and non-local addresses"
>> 319 " daddr=%pI4\n", &dest->addr.ip);
>> ^^^^^^^^^^^^^
>
> Good catch. In fact, I requested Alex to print
> daddr in this case but then we both missed this dest here...
> Alex, both v4 (&daddr) and v6 (daddr) should be fixed.
It doesn't appear to me that there's been a patch for this yet. I'll
write one and send it out right away. Please LMK if there's anyone else
I should send it to.
Thanks,
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-01 17:45 ipvs: Pull out crosses_local_route_boundary logic Dan Carpenter
@ 2014-10-06 0:54 ` Alex Gartrell
2014-10-06 0:54 ` Alex Gartrell
1 sibling, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 0:54 UTC (permalink / raw)
To: horms; +Cc: ja, dan.carpenter, lvs-devel, netdev, kernel-team, Alex Gartrell
Ensure that the pointer is non-NULL before dereferencing it for debugging
purposes.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..06bba9b 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI4\n", &dest->addr.ip);
+ " daddr=%pI4\n", dest ? &dest->addr.ip : NULL);
goto err_put;
}
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI6\n", &dest->addr.in6);
+ " daddr=%pI6\n", dest ? &dest->addr.in6 : NULL);
goto err_put;
}
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
@ 2014-10-06 0:54 ` Alex Gartrell
0 siblings, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 0:54 UTC (permalink / raw)
To: horms; +Cc: ja, dan.carpenter, lvs-devel, netdev, kernel-team, Alex Gartrell
Ensure that the pointer is non-NULL before dereferencing it for debugging
purposes.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..06bba9b 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI4\n", &dest->addr.ip);
+ " daddr=%pI4\n", dest ? &dest->addr.ip : NULL);
goto err_put;
}
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI6\n", &dest->addr.in6);
+ " daddr=%pI6\n", dest ? &dest->addr.in6 : NULL);
goto err_put;
}
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 0:54 ` Alex Gartrell
(?)
@ 2014-10-06 6:49 ` Julian Anastasov
2014-10-06 15:56 ` Alex Gartrell
-1 siblings, 1 reply; 15+ messages in thread
From: Julian Anastasov @ 2014-10-06 6:49 UTC (permalink / raw)
To: Alex Gartrell; +Cc: horms, dan.carpenter, lvs-devel, netdev, kernel-team
Hello,
On Sun, 5 Oct 2014, Alex Gartrell wrote:
> Ensure that the pointer is non-NULL before dereferencing it for debugging
> purposes.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Alex Gartrell <agartrell@fb.com>
> ---
> net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 91f17c1..06bba9b 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> local))) {
> IP_VS_DBG_RL("We are crossing local and non-local addresses"
> - " daddr=%pI4\n", &dest->addr.ip);
> + " daddr=%pI4\n", dest ? &dest->addr.ip : NULL);
> goto err_put;
> }
>
> @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> local))) {
> IP_VS_DBG_RL("We are crossing local and non-local addresses"
> - " daddr=%pI6\n", &dest->addr.in6);
> + " daddr=%pI6\n", dest ? &dest->addr.in6 : NULL);
> goto err_put;
> }
You have to print the "daddr" variable as
it was done before your patchset in the
"Stopping traffic to %s address, dest: %p..." message
because dest is not present in all cases, for example,
for *bypass_xmit. Other places provide cp->daddr but
for backup server some conns can live without cp->dest.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ipvs: Pull out crosses_local_route_boundary logic
2014-10-05 23:47 ` Alex Gartrell
@ 2014-10-06 7:32 ` Simon Horman
2014-10-06 15:45 ` Alex Gartrell
2014-10-06 15:46 ` Alex Gartrell
0 siblings, 2 replies; 15+ messages in thread
From: Simon Horman @ 2014-10-06 7:32 UTC (permalink / raw)
To: Alex Gartrell; +Cc: Julian Anastasov, Dan Carpenter, lvs-devel
On Sun, Oct 05, 2014 at 04:47:02PM -0700, Alex Gartrell wrote:
> Hey everyone,
>
> On 10/1/14 12:37 PM, Julian Anastasov wrote:
> >> 312 *ret_saddr = saddr;
> >> 313 }
> >> 314
> >> 315 local = (rt->rt_flags & RTCF_LOCAL) ? 1 : 0;
> >> 316 if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> >> 317 local))) {
> >> 318 IP_VS_DBG_RL("We are crossing local and non-local addresses"
> >> 319 " daddr=%pI4\n", &dest->addr.ip);
> >> ^^^^^^^^^^^^^
> >
> > Good catch. In fact, I requested Alex to print
> >daddr in this case but then we both missed this dest here...
> >Alex, both v4 (&daddr) and v6 (daddr) should be fixed.
>
> It doesn't appear to me that there's been a patch for this yet. I'll write
> one and send it out right away. Please LMK if there's anyone else I should
> send it to.
I am following this thread so its not a big deal but in general please CC
me as well as Julian and lvs-devel on any patches you want considered for
inclusion in IPVS kernel code.
Thanks
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ipvs: Pull out crosses_local_route_boundary logic
2014-10-06 7:32 ` Simon Horman
@ 2014-10-06 15:45 ` Alex Gartrell
2014-10-06 15:46 ` Alex Gartrell
1 sibling, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 15:45 UTC (permalink / raw)
To: Simon Horman; +Cc: Julian Anastasov, Dan Carpenter, lvs-devel
Hey Simon,
On 10/06/2014 12:32 AM, Simon Horman wrote:
> I am following this thread so its not a big deal but in general please CC
> me as well as Julian and lvs-devel on any patches you want considered for
> inclusion in IPVS kernel code.
>
Just double checking (due to previous mail-related issues), did you get
the patch I sent last night? You were to'ed there.
I've just mailed another patch using daddr that should address this issue.
Thanks,
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 7:32 ` Simon Horman
@ 2014-10-06 15:46 ` Alex Gartrell
2014-10-06 15:46 ` Alex Gartrell
1 sibling, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 15:46 UTC (permalink / raw)
To: horms; +Cc: ja, dan.carpenter, lvs-devel, netdev, kernel-team, Alex Gartrell
Use daddr instead of reaching into dest.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..437a366 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI4\n", &dest->addr.ip);
+ " daddr=%pI4\n", &daddr);
goto err_put;
}
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI6\n", &dest->addr.in6);
+ " daddr=%pI6\n", daddr);
goto err_put;
}
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
@ 2014-10-06 15:46 ` Alex Gartrell
0 siblings, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 15:46 UTC (permalink / raw)
To: horms; +Cc: ja, dan.carpenter, lvs-devel, netdev, kernel-team, Alex Gartrell
Use daddr instead of reaching into dest.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..437a366 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI4\n", &dest->addr.ip);
+ " daddr=%pI4\n", &daddr);
goto err_put;
}
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
local))) {
IP_VS_DBG_RL("We are crossing local and non-local addresses"
- " daddr=%pI6\n", &dest->addr.in6);
+ " daddr=%pI6\n", daddr);
goto err_put;
}
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 6:49 ` Julian Anastasov
@ 2014-10-06 15:56 ` Alex Gartrell
0 siblings, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 15:56 UTC (permalink / raw)
To: Julian Anastasov; +Cc: horms, dan.carpenter, lvs-devel, netdev, kernel-team
Hey Julian,
On 10/05/2014 11:49 PM, Julian Anastasov wrote:
>
> You have to print the "daddr" variable as
> it was done before your patchset in the
> "Stopping traffic to %s address, dest: %p..." message
> because dest is not present in all cases, for example,
> for *bypass_xmit. Other places provide cp->daddr but
> for backup server some conns can live without cp->dest.
I've sent an updated patch that does this but I have some questions
about other stuff that I find mildly confusing. Specifically I didn't
realize until looking at the call sites that !dest || daddr =
dest->addr.ip (but maybe I'm wrong?)
If that's the case, why do we have the following line in __ip_vs_get_out_rt?
daddr = dest->addr.ip;
If that's /always/ true then we should add the following line or a
comment to the same effect to clarify
BUG_ON(dest && dest->addr.ip != daddr);
If that's not intended to always be true, then should the patch be the
following?
...%pI4", dest ? &dest->addr.ip : &daddr);
Thanks,
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
@ 2014-10-06 15:56 ` Alex Gartrell
0 siblings, 0 replies; 15+ messages in thread
From: Alex Gartrell @ 2014-10-06 15:56 UTC (permalink / raw)
To: Julian Anastasov; +Cc: horms, dan.carpenter, lvs-devel, netdev, kernel-team
Hey Julian,
On 10/05/2014 11:49 PM, Julian Anastasov wrote:
>
> You have to print the "daddr" variable as
> it was done before your patchset in the
> "Stopping traffic to %s address, dest: %p..." message
> because dest is not present in all cases, for example,
> for *bypass_xmit. Other places provide cp->daddr but
> for backup server some conns can live without cp->dest.
I've sent an updated patch that does this but I have some questions
about other stuff that I find mildly confusing. Specifically I didn't
realize until looking at the call sites that !dest || daddr =
dest->addr.ip (but maybe I'm wrong?)
If that's the case, why do we have the following line in __ip_vs_get_out_rt?
daddr = dest->addr.ip;
If that's /always/ true then we should add the following line or a
comment to the same effect to clarify
BUG_ON(dest && dest->addr.ip != daddr);
If that's not intended to always be true, then should the patch be the
following?
...%pI4", dest ? &dest->addr.ip : &daddr);
Thanks,
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 15:46 ` Alex Gartrell
(?)
@ 2014-10-06 19:01 ` Julian Anastasov
2014-10-17 6:27 ` Simon Horman
-1 siblings, 1 reply; 15+ messages in thread
From: Julian Anastasov @ 2014-10-06 19:01 UTC (permalink / raw)
To: Alex Gartrell; +Cc: horms, dan.carpenter, lvs-devel, netdev, kernel-team
Hello,
On Mon, 6 Oct 2014, Alex Gartrell wrote:
> Use daddr instead of reaching into dest.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Alex Gartrell <agartrell@fb.com>
Thanks!
Acked-by: Julian Anastasov <ja@ssi.bg>
> ---
> net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 91f17c1..437a366 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> local))) {
> IP_VS_DBG_RL("We are crossing local and non-local addresses"
> - " daddr=%pI4\n", &dest->addr.ip);
> + " daddr=%pI4\n", &daddr);
> goto err_put;
> }
>
> @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> local))) {
> IP_VS_DBG_RL("We are crossing local and non-local addresses"
> - " daddr=%pI6\n", &dest->addr.in6);
> + " daddr=%pI6\n", daddr);
> goto err_put;
> }
>
> --
> Alex Gartrell <agartrell@fb.com>
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 15:56 ` Alex Gartrell
(?)
@ 2014-10-06 19:13 ` Julian Anastasov
-1 siblings, 0 replies; 15+ messages in thread
From: Julian Anastasov @ 2014-10-06 19:13 UTC (permalink / raw)
To: Alex Gartrell; +Cc: horms, dan.carpenter, lvs-devel, netdev, kernel-team
Hello,
On Mon, 6 Oct 2014, Alex Gartrell wrote:
> Hey Julian,
>
> I've sent an updated patch that does this but I have some questions about
> other stuff that I find mildly confusing. Specifically I didn't realize until
> looking at the call sites that !dest || daddr = dest->addr.ip (but maybe I'm
> wrong?)
>
> If that's the case, why do we have the following line in __ip_vs_get_out_rt?
>
> daddr = dest->addr.ip;
Extra line that is not needed...
> If that's /always/ true then we should add the following line or a comment to
> the same effect to clarify
>
> BUG_ON(dest && dest->addr.ip != daddr);
IMHO, BUG_ON is not needed.
> If that's not intended to always be true, then should the patch be the
> following?
>
> ...%pI4", dest ? &dest->addr.ip : &daddr);
Using daddr is fine.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code
2014-10-06 19:01 ` Julian Anastasov
@ 2014-10-17 6:27 ` Simon Horman
0 siblings, 0 replies; 15+ messages in thread
From: Simon Horman @ 2014-10-17 6:27 UTC (permalink / raw)
To: Julian Anastasov
Cc: Alex Gartrell, dan.carpenter, lvs-devel, netdev, kernel-team
On Mon, Oct 06, 2014 at 10:01:08PM +0300, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 6 Oct 2014, Alex Gartrell wrote:
>
> > Use daddr instead of reaching into dest.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Alex Gartrell <agartrell@fb.com>
>
> Thanks!
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Thanks, I have applied this to the ipvs tree and will see about
both getting it included in v3.18 and v3.17-stable.
> > ---
> > net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> > index 91f17c1..437a366 100644
> > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> > if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> > local))) {
> > IP_VS_DBG_RL("We are crossing local and non-local addresses"
> > - " daddr=%pI4\n", &dest->addr.ip);
> > + " daddr=%pI4\n", &daddr);
> > goto err_put;
> > }
> >
> > @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> > if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> > local))) {
> > IP_VS_DBG_RL("We are crossing local and non-local addresses"
> > - " daddr=%pI6\n", &dest->addr.in6);
> > + " daddr=%pI6\n", daddr);
> > goto err_put;
> > }
> >
> > --
> > Alex Gartrell <agartrell@fb.com>
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-10-17 6:27 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-01 17:45 ipvs: Pull out crosses_local_route_boundary logic Dan Carpenter
2014-10-01 19:37 ` Julian Anastasov
2014-10-05 23:47 ` Alex Gartrell
2014-10-06 7:32 ` Simon Horman
2014-10-06 15:45 ` Alex Gartrell
2014-10-06 15:46 ` [PATCH net-next] ipvs: Avoid null-pointer deref in debug code Alex Gartrell
2014-10-06 15:46 ` Alex Gartrell
2014-10-06 19:01 ` Julian Anastasov
2014-10-17 6:27 ` Simon Horman
2014-10-06 0:54 ` Alex Gartrell
2014-10-06 0:54 ` Alex Gartrell
2014-10-06 6:49 ` Julian Anastasov
2014-10-06 15:56 ` Alex Gartrell
2014-10-06 15:56 ` Alex Gartrell
2014-10-06 19:13 ` Julian Anastasov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.