Re: LVS in 2.4.23: crash in ip_vs_conn_unhash

["Brett E." <brettspamacct@xxxxxxxxxxxxx>]

Thanks, I can reproduce it easily, in fact it just happened to the 
backup test machine this morning.  I reproduce it by redirecting 1000 
connections/sec load from our production servers via a network tap to 2 
LVS machines running keepalived in primary-backup mode so it's stressing 
it quite a bit. I'll apply this then tell you how it goes.
Julian Anastasov wrote:

>         Hello,
>
> On Thu, 11 Mar 2004, Brett E. wrote:
>
>
> > I see that ip_vs_conn_unhash has been changed from ipvs in 2.4.23 to the
> > ipvs in 2.6.2 so is this a known bug that was fixed?
>
>         No, 2.6 does the same.
>
>
> > I'm using LVS from 2.4.23 on a 2 way SMP machine and kdb crashes in
> > ip_vs_check_template, I'm leaving kdb up and running, but here's the
> > data I transribed by hand, hopefully I didn't make any typos:
>
>         Is it happening often, can you reproduce it with the attached
> patch? It changes some places that use conn_unhash.
>
> Regards
>
> --
> Julian Anastasov <ja@xxxxxx>
>
>
> ------------------------------------------------------------------------
>
> diff -ur v2.4.25/linux/net/ipv4/ipvs/ip_vs_conn.c 
> linux/net/ipv4/ipvs/ip_vs_conn.c
> --- v2.4.25/linux/net/ipv4/ipvs/ip_vs_conn.c    2003-11-28 22:04:14.000000000 
> +0200
> +++ linux/net/ipv4/ipvs/ip_vs_conn.c    2004-03-13 11:23:34.598986048 +0200
> @@ -142,20 +142,19 @@
>  {
>         unsigned hash;
>  
> -	if (cp->flags & IP_VS_CONN_F_HASHED) {
> -               IP_VS_ERR("ip_vs_conn_hash(): request for already hashed, "
> -                         "called from %p\n", __builtin_return_address(0));
> -               return 0;
> -       }
> -
>         /* Hash by protocol, client address and port */
>         hash = ip_vs_conn_hashkey(cp->protocol, cp->caddr, cp->cport);
>  
>  	ct_write_lock(hash);
>  
> -	list_add(&cp->c_list, &ip_vs_conn_tab[hash]);
> -       cp->flags |= IP_VS_CONN_F_HASHED;
> -       atomic_inc(&cp->refcnt);
> +       if (!(cp->flags & IP_VS_CONN_F_HASHED)) {
> +               list_add(&cp->c_list, &ip_vs_conn_tab[hash]);
> +               cp->flags |= IP_VS_CONN_F_HASHED;
> +               atomic_inc(&cp->refcnt);
> +       } else {
> +               IP_VS_ERR("ip_vs_conn_hash(): request for already hashed, "
> +                         "called from %p\n", __builtin_return_address(0));
> +       }
>  
>  	ct_write_unlock(hash);
>  
> @@ -170,24 +169,23 @@
>  static int ip_vs_conn_unhash(struct ip_vs_conn *cp)
>  {
>         unsigned hash;
> -
> -       if (!(cp->flags & IP_VS_CONN_F_HASHED)) {
> -               IP_VS_ERR("ip_vs_conn_unhash(): request for unhash flagged, "
> -                         "called from %p\n", __builtin_return_address(0));
> -               return 0;
> -       }
> +       int ret;
>  
>  	/* unhash it and decrease its reference counter */
>         hash = ip_vs_conn_hashkey(cp->protocol, cp->caddr, cp->cport);
>         ct_write_lock(hash);
>  
> -	list_del(&cp->c_list);
> -       cp->flags &= ~IP_VS_CONN_F_HASHED;
> -       atomic_dec(&cp->refcnt);
> +       if (cp->flags & IP_VS_CONN_F_HASHED) {
> +               list_del(&cp->c_list);
> +               cp->flags &= ~IP_VS_CONN_F_HASHED;
> +               atomic_dec(&cp->refcnt);
> +               ret = 1;
> +       } else
> +               ret = 0;
>  
>  	ct_write_unlock(hash);
>  
> -	return 1;
> +       return ret;
>  }
>  
>  
> @@ -726,14 +724,16 @@
>          *  Check if it is no_cport connection ...
>          */
>         if (cp->flags & IP_VS_CONN_F_NO_CPORT) {
> -               atomic_dec(&ip_vs_conn_no_cport_cnt);
> -               ip_vs_conn_unhash(cp);
> -               cp->flags &= ~IP_VS_CONN_F_NO_CPORT;
> -               cp->cport = h.portp[0];
> -               /* hash on new dport */
> -               ip_vs_conn_hash(cp);
> -
> -               IP_VS_DBG(10, "filled cport=%d\n", ntohs(cp->dport));
> +               if (ip_vs_conn_unhash(cp)) {
> +                       if (cp->flags & IP_VS_CONN_F_NO_CPORT) {
> +                               atomic_dec(&ip_vs_conn_no_cport_cnt);
> +                               cp->flags &= ~IP_VS_CONN_F_NO_CPORT;
> +                               cp->cport = h.portp[0];
> +                               IP_VS_DBG(10, "filled cport=%d\n", 
> ntohs(cp->dport));
> +                       }
> +                       /* hash on new dport */
> +                       ip_vs_conn_hash(cp);
> +               }
>         }
>  
>  	if (!(rt = __ip_vs_get_out_rt(cp, RT_TOS(iph->tos))))
> @@ -1142,11 +1142,12 @@
>                 /*
>                  * Invalidate the connection template
>                  */
> -               ip_vs_conn_unhash(ct);
> -               ct->dport = 65535;
> -               ct->vport = 65535;
> -               ct->cport = 0;
> -               ip_vs_conn_hash(ct);
> +               if (ct->cport && ip_vs_conn_unhash(ct)) {
> +                       ct->dport = 65535;
> +                       ct->vport = 65535;
> +                       ct->cport = 0;
> +                       ip_vs_conn_hash(ct);
> +               }
>  
>  		/*
>                  * Simply decrease the refcnt of the template,
> @@ -1200,7 +1201,8 @@
>         /*
>          *      unhash it if it is hashed in the conn table
>          */
> -       ip_vs_conn_unhash(cp);
> +       if (!ip_vs_conn_unhash(cp))
> +               goto expire_later;
>  
>  	/*
>          *      refcnt==1 implies I'm the only one referrer