re: ipvs: Pull out crosses_local_route_boundary logic

re: ipvs: Pull out crosses_local_route_boundary logic

[Dan Carpenter]

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

re: ipvs: Pull out crosses_local_route_boundary logic

[Julian Anastasov]

	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>

Re: ipvs: Pull out crosses_local_route_boundary logic

[Alex Gartrell]

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>

Re: ipvs: Pull out crosses_local_route_boundary logic

[Simon Horman]

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

Re: ipvs: Pull out crosses_local_route_boundary logic

[Alex Gartrell]

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>

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[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>

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Simon Horman]

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>
> 

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[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>

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Alex Gartrell]

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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Alex Gartrell]

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>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	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>