LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH net-next 1/2] ipvs: add assured state for conn templates

To: Julian Anastasov <ja@xxxxxx>
Subject: Re: [PATCH net-next 1/2] ipvs: add assured state for conn templates
Cc: lvs-devel@xxxxxxxxxxxxxxx, mkoutny@xxxxxxxx, mkubecek@xxxxxxxx
From: Simon Horman <horms@xxxxxxxxxxxx>
Date: Mon, 4 Jun 2018 04:00:48 -0400
On Sat, Jun 02, 2018 at 09:50:01PM +0300, Julian Anastasov wrote:
> cp->state was not used for templates. Add support for state bits
> and for the first "assured" bit which indicates that some
> connection controlled by this template was established or assured
> by the real server. In a followup patch we will use it to drop
> templates under SYN attack.
> 
> Signed-off-by: Julian Anastasov <ja@xxxxxx>
> ---
>  include/net/ip_vs.h                   |  7 ++++++-
>  net/netfilter/ipvs/ip_vs_conn.c       |  8 ++++----
>  net/netfilter/ipvs/ip_vs_proto.c      | 19 ++++++++++++++++---
>  net/netfilter/ipvs/ip_vs_proto_sctp.c |  7 +++++++
>  net/netfilter/ipvs/ip_vs_proto_tcp.c  |  7 +++++++
>  net/netfilter/ipvs/ip_vs_proto_udp.c  |  7 +++++++
>  net/netfilter/ipvs/ip_vs_sync.c       | 18 ++++++++----------
>  7 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 824d7ef..d786649 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -347,6 +347,11 @@ enum ip_vs_sctp_states {
>       IP_VS_SCTP_S_LAST
>  };
>  
> +/* Connection templates use bits from state */
> +#define IP_VS_CTPL_S_NONE            0x0000
> +#define IP_VS_CTPL_S_ASSURED         0x0001
> +#define IP_VS_CTPL_S_LAST            0x0002
> +
>  /* Delta sequence info structure
>   * Each ip_vs_conn has 2 (output AND input seq. changes).
>   * Only used in the VS/NAT.
> @@ -1232,7 +1237,7 @@ struct ip_vs_conn *ip_vs_conn_new(const struct 
> ip_vs_conn_param *p, int dest_af,
>                                 struct ip_vs_dest *dest, __u32 fwmark);
>  void ip_vs_conn_expire_now(struct ip_vs_conn *cp);
>  
> -const char *ip_vs_state_name(__u16 proto, int state);
> +const char *ip_vs_state_name(const struct ip_vs_conn *cp);
>  
>  void ip_vs_tcp_conn_listen(struct ip_vs_conn *cp);
>  int ip_vs_check_template(struct ip_vs_conn *ct, struct ip_vs_dest *cdest);
> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 75de465..8f76644 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1107,7 +1107,7 @@ static int ip_vs_conn_seq_show(struct seq_file *seq, 
> void *v)
>                               &cp->caddr.in6, ntohs(cp->cport),
>                               &cp->vaddr.in6, ntohs(cp->vport),
>                               dbuf, ntohs(cp->dport),
> -                             ip_vs_state_name(cp->protocol, cp->state),
> +                             ip_vs_state_name(cp),
>                               (cp->timer.expires-jiffies)/HZ, pe_data);
>               else
>  #endif
> @@ -1118,7 +1118,7 @@ static int ip_vs_conn_seq_show(struct seq_file *seq, 
> void *v)
>                               ntohl(cp->caddr.ip), ntohs(cp->cport),
>                               ntohl(cp->vaddr.ip), ntohs(cp->vport),
>                               dbuf, ntohs(cp->dport),
> -                             ip_vs_state_name(cp->protocol, cp->state),
> +                             ip_vs_state_name(cp),
>                               (cp->timer.expires-jiffies)/HZ, pe_data);
>       }
>       return 0;
> @@ -1182,7 +1182,7 @@ static int ip_vs_conn_sync_seq_show(struct seq_file 
> *seq, void *v)
>                               &cp->caddr.in6, ntohs(cp->cport),
>                               &cp->vaddr.in6, ntohs(cp->vport),
>                               dbuf, ntohs(cp->dport),
> -                             ip_vs_state_name(cp->protocol, cp->state),
> +                             ip_vs_state_name(cp),
>                               ip_vs_origin_name(cp->flags),
>                               (cp->timer.expires-jiffies)/HZ);
>               else
> @@ -1194,7 +1194,7 @@ static int ip_vs_conn_sync_seq_show(struct seq_file 
> *seq, void *v)
>                               ntohl(cp->caddr.ip), ntohs(cp->cport),
>                               ntohl(cp->vaddr.ip), ntohs(cp->vport),
>                               dbuf, ntohs(cp->dport),
> -                             ip_vs_state_name(cp->protocol, cp->state),
> +                             ip_vs_state_name(cp),
>                               ip_vs_origin_name(cp->flags),
>                               (cp->timer.expires-jiffies)/HZ);
>       }
> diff --git a/net/netfilter/ipvs/ip_vs_proto.c 
> b/net/netfilter/ipvs/ip_vs_proto.c
> index ca880a3..ae7d786 100644
> --- a/net/netfilter/ipvs/ip_vs_proto.c
> +++ b/net/netfilter/ipvs/ip_vs_proto.c
> @@ -42,6 +42,11 @@
>  
>  static struct ip_vs_protocol *ip_vs_proto_table[IP_VS_PROTO_TAB_SIZE];
>  
> +/* States for conn templates: NONE or letters separated with "," */
> +static const char *ip_vs_ctpl_state_name_table[IP_VS_CTPL_S_LAST] = {
> +     [IP_VS_CTPL_S_NONE]                     = "NONE",
> +     [IP_VS_CTPL_S_ASSURED]                  = "A",

"A" seems a bit cryptic, why not "ASSURED" ?

> +};
>  
>  /*
>   *   register an ipvs protocol
> @@ -193,12 +198,20 @@ ip_vs_create_timeout_table(int *table, int size)
>  }
>  
>  
> -const char * ip_vs_state_name(__u16 proto, int state)
> +const char *ip_vs_state_name(const struct ip_vs_conn *cp)
>  {
> -     struct ip_vs_protocol *pp = ip_vs_proto_get(proto);
> +     unsigned int state = cp->state;
> +     struct ip_vs_protocol *pp;
> +
> +     if (cp->flags & IP_VS_CONN_F_TEMPLATE) {
>  
> +             if (state >= IP_VS_CTPL_S_LAST)
> +                     return "ERR!";
> +             return ip_vs_ctpl_state_name_table[state] ? : "?";
> +     }
> +     pp = ip_vs_proto_get(cp->protocol);
>       if (pp == NULL || pp->state_name == NULL)
> -             return (IPPROTO_IP == proto) ? "NONE" : "ERR!";
> +             return (cp->protocol == IPPROTO_IP) ? "NONE" : "ERR!";
>       return pp->state_name(state);
>  }

I'd slightly prefer if refactoring ip_vs_state_name to not take a state
parameter was a separate patch. Its a nice cleanup. But it doesn't seem
related to the rest of what is going on here.

>  
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c 
> b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 3250c4a1..13826ee 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -461,6 +461,13 @@ set_sctp_state(struct ip_vs_proto_data *pd, struct 
> ip_vs_conn *cp,
>                               cp->flags &= ~IP_VS_CONN_F_INACTIVE;
>                       }
>               }
> +             if (next_state == IP_VS_SCTP_S_ESTABLISHED) {
> +                     struct ip_vs_conn *ct = cp->control;
> +
> +                     if (ct && (ct->flags & IP_VS_CONN_F_TEMPLATE) &&
> +                         !(ct->state & IP_VS_CTPL_S_ASSURED))
> +                             ct->state |= IP_VS_CTPL_S_ASSURED;
> +             }

The logic above seems to be replicated several times below.
Could we have a helper function?

>       }
>       if (likely(pd))
>               cp->timeout = pd->timeout_table[cp->state = next_state];
> diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c 
> b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> index b73fe6e..467a69e 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> @@ -569,6 +569,13 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct 
> ip_vs_conn *cp,
>                               cp->flags &= ~IP_VS_CONN_F_INACTIVE;
>                       }
>               }
> +             if (new_state == IP_VS_TCP_S_ESTABLISHED) {
> +                     struct ip_vs_conn *ct = cp->control;
> +
> +                     if (ct && (ct->flags & IP_VS_CONN_F_TEMPLATE) &&
> +                         !(ct->state & IP_VS_CTPL_S_ASSURED))
> +                             ct->state |= IP_VS_CTPL_S_ASSURED;
> +             }
>       }
>  
>       if (likely(pd))
> diff --git a/net/netfilter/ipvs/ip_vs_proto_udp.c 
> b/net/netfilter/ipvs/ip_vs_proto_udp.c
> index e0ef11c..c733ed6 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_udp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_udp.c
> @@ -460,6 +460,13 @@ udp_state_transition(struct ip_vs_conn *cp, int 
> direction,
>       }
>  
>       cp->timeout = pd->timeout_table[IP_VS_UDP_S_NORMAL];
> +     if (direction == IP_VS_DIR_OUTPUT) {
> +             struct ip_vs_conn *ct = cp->control;
> +
> +             if (ct && (ct->flags & IP_VS_CONN_F_TEMPLATE) &&
> +                 !(ct->state & IP_VS_CTPL_S_ASSURED))
> +                     ct->state |= IP_VS_CTPL_S_ASSURED;
> +     }
>  }
>  
>  static int __udp_init(struct netns_ipvs *ipvs, struct ip_vs_proto_data *pd)
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 001501e..24891bd 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -1003,11 +1003,10 @@ static void ip_vs_process_message_v0(struct 
> netns_ipvs *ipvs, const char *buffer
>                               continue;
>                       }
>               } else {
> -                     /* protocol in templates is not used for state/timeout 
> */
> -                     if (state > 0) {
> -                             IP_VS_DBG(2, "BACKUP v0, Invalid template state 
> %u\n",
> -                                     state);
> -                             state = 0;
> +                     if (state >= IP_VS_CTPL_S_LAST) {
> +                             IP_VS_DBG(7, "BACKUP v0, Invalid tpl state 
> %u\n",
> +                                       state);

Not strictly related to this patch, but should these debug messages
be rate limited in some way?

> +                             state &= IP_VS_CTPL_S_LAST - 1;
>                       }
>               }
>  
> @@ -1166,11 +1165,10 @@ static inline int ip_vs_proc_sync_conn(struct 
> netns_ipvs *ipvs, __u8 *p, __u8 *m
>                       goto out;
>               }
>       } else {
> -             /* protocol in templates is not used for state/timeout */
> -             if (state > 0) {
> -                     IP_VS_DBG(3, "BACKUP, Invalid template state %u\n",
> -                             state);
> -                     state = 0;
> +             if (state >= IP_VS_CTPL_S_LAST) {
> +                     IP_VS_DBG(7, "BACKUP, Invalid tpl state %u\n",
> +                               state);
> +                     state &= IP_VS_CTPL_S_LAST - 1;
>               }
>       }
>       if (ip_vs_conn_fill_param_sync(ipvs, af, s, &param, pe_data,
> -- 
> 2.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>