Re: [PATCH] netfilter/ipvs: expire no destination UDP connections when expire_nodest_conn=1

[Andrew Kim <kim.andrewsy@xxxxxxxxx>]

Hi Julian,

Thanks for getting back to me, that makes sense.

Would you be opposed to trying to expire all UDP connections matching
a deleted destination only if expire_nodest_conn=1?
Even today with `expire_nodest_conn=1`, many packets could be dropped
if there are many requests from a single client
trying to reuse client ports matching a deleted destination. Setting
`expire_nodest_conn=1` and reducing the UDP timeout
helps but deleting all connections when the destination is deleted
seems more efficient.

Looking forward to hearing your thoughts,

Andrew Sy Kim


On Fri, May 15, 2020 at 2:07 PM Julian Anastasov <ja@xxxxxx> wrote:
>
>
>         Hello,
>
> On Thu, 14 May 2020, Andrew Sy Kim wrote:
>
> > When expire_nodest_conn=1 and an IPVS destination is deleted, IPVS
> > doesn't expire connections with the IP_VS_CONN_F_ONE_PACKET flag set (any
> > UDP connection). If there are many UDP packets to a virtual server from a
> > single client and a destination is deleted, many packets are silently
> > dropped whenever an existing connection entry with the same source port
> > exists. This patch ensures IPVS also expires UDP connections when a
> > packet matches an existing connection with no destinations.
> >
> > Signed-off-by: Andrew Sy Kim <kim.andrewsy@xxxxxxxxx>
> > ---
> >  net/netfilter/ipvs/ip_vs_core.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c 
> > b/net/netfilter/ipvs/ip_vs_core.c
> > index aa6a603a2425..f0535586fe75 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -2116,8 +2116,7 @@ ip_vs_in(struct netns_ipvs *ipvs, unsigned int 
> > hooknum, struct sk_buff *skb, int
> >               else
> >                       ip_vs_conn_put(cp);
>
>         Above ip_vs_conn_put() should free the ONE_PACKET
> connections because:
>
> - such connections never start timer, they are designed
> to exist just to schedule the packet, then they are released.
> - noone takes extra references
>
>         So, ip_vs_conn_put() simply calls ip_vs_conn_expire()
> where connections should be released immediately. As result,
> we can not access cp after this point here. That is why we work
> just with 'flags' below...
>
>         Note that not every UDP connection has ONE_PACKET
> flag, it is present if you configure it for the service.
> Do you have -o/--ops flag? If not, the UDP connection
> should expire before the next jiffie. This is the theory,
> in practice, you may observe some problem...
>
> > -             if (sysctl_expire_nodest_conn(ipvs) &&
> > -                 !(flags & IP_VS_CONN_F_ONE_PACKET)) {
> > +             if (sysctl_expire_nodest_conn(ipvs)) {
> >                       /* try to expire the connection immediately */
> >                       ip_vs_conn_expire_now(cp);
> >               }
>
>         You can also look at the discussion which resulted in
> the last patch for this place:
>
> http://archive.linuxvirtualserver.org/html/lvs-devel/2018-07/msg00014.html
>
> Regards
>
> --
> Julian Anastasov <ja@xxxxxx>