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

[Andrew Kim <kim.andrewsy@xxxxxxxxx>]

I sent a new patch diff based on my ask above. Please take a look :)

Thanks,

Andrew Sy Kim

On Sun, May 17, 2020 at 1:27 PM Andrew Kim <kim.andrewsy@xxxxxxxxx> wrote:
>
> 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>