LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH] IPVS: Move userspace definitions to include/linux/ip_vs.h

To: Simon Horman <horms@xxxxxxxxxxxx>
Subject: Re: [PATCH] IPVS: Move userspace definitions to include/linux/ip_vs.h
Cc: Julius Volz <juliusv@xxxxxxxxxx>, lvs-devel@xxxxxxxxxxxxxxx, netdev@xxxxxxxxxxxxxxx, davem@xxxxxxxxxxxxx, vbusam@xxxxxxxxxx
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Thu, 24 Jul 2008 13:22:34 +0200
Simon Horman wrote:
On Thu, Jul 24, 2008 at 12:14:49PM +0200, Julius Volz wrote:
Current versions of ipvsadm include "/usr/src/linux/include/net/ip_vs.h"
directly. This file also contains kernel-only definitions. Normally, public
definitions should live in include/linux, so this patch moves the
definitions shared with userspace to a new file, "include/linux/ip_vs.h".

To make old ipvsadms still compile with this, the old header file includes
the new one.

Thanks to Dave Miller and Horms for noting/adding the missing Kbuild entry
for the new header file.

Signed-off-by: Julius Volz <juliusv@xxxxxxxxxx>

Acked-by: Simon Horman <horms@xxxxxxxxxxxx>

---
+/* Move it to better place one day, for now keep it unique */
+#define NFC_IPVS_PROPERTY      0x10000

Does this have any connection to the skb flag? If so, does
it really belong in the userspace interface?

+struct ip_vs_service_user {
+       /* virtual service addresses */
+       u_int16_t               protocol;
+       __be32                  addr;           /* virtual ip address */
+       __be16                  port;

If you switch the above two you plug two holes in the struct.

+       u_int32_t               fwmark;         /* firwall mark of service */
+
+       /* virtual service options */
+       char                    sched_name[IP_VS_SCHEDNAME_MAXLEN];
+       unsigned                flags;          /* virtual service flags */
+       unsigned                timeout;        /* persistent timeout in sec */
+       __be32                  netmask;        /* persistent netmask */
+};
+
+
+struct ip_vs_dest_user {
+       /* destination server address */
+       __be32                  addr;
+       __be16                  port;

This also leaves a hole, you should make sure its zeroed explicitly
to avoid leaking information to userspace.

+
+       /* real server options */
+       unsigned                conn_flags;     /* connection flags */
+       int                     weight;         /* destination weight */
+
+       /* thresholds for active connections */
+       u_int32_t               u_threshold;    /* upper threshold */
+       u_int32_t               l_threshold;    /* lower threshold */
+};
+
+
+/*
+ *     IPVS statistics object (for user space)
+ */
+struct ip_vs_stats_user
+{
+       __u32                   conns;          /* connections scheduled */
+       __u32                   inpkts;         /* incoming packets */
+       __u32                   outpkts;        /* outgoing packets */
+       __u64                   inbytes;        /* incoming bytes */
+       __u64                   outbytes;       /* outgoing bytes */

Putting the u64s first would also plug a hole.

+
+       __u32                   cps;            /* current connection rate */
+       __u32                   inpps;          /* current in packet rate */
+       __u32                   outpps;         /* current out packet rate */
+       __u32                   inbps;          /* current in byte rate */
+       __u32                   outbps;         /* current out byte rate */
+};
+
+
+/* The argument to IP_VS_SO_GET_INFO */
+struct ip_vs_getinfo {
+       /* version number */
+       unsigned int            version;
+
+       /* size of connection hash table */
+       unsigned int            size;
+
+       /* number of virtual services */
+       unsigned int            num_services;
+};
+
+
+/* The argument to IP_VS_SO_GET_SERVICE */
+struct ip_vs_service_entry {
+       /* which service: user fills in these */
+       u_int16_t               protocol;
+       __be32                  addr;           /* virtual address */
+       __be16                  port;

Same as above.

+       u_int32_t               fwmark;         /* firwall mark of service */
+
+       /* service options */
+       char                    sched_name[IP_VS_SCHEDNAME_MAXLEN];
+       unsigned                flags;          /* virtual service flags */
+       unsigned                timeout;        /* persistent timeout */
+       __be32                  netmask;        /* persistent netmask */
+
+       /* number of real servers */
+       unsigned int            num_dests;
+
+       /* statistics */
+       struct ip_vs_stats_user stats;
+};
+
+
+struct ip_vs_dest_entry {
+       __be32                  addr;           /* destination address */
+       __be16                  port;

Also a hole that should be cleared.

+       unsigned                conn_flags;     /* connection flags */
+       int                     weight;         /* destination weight */
+
+       u_int32_t               u_threshold;    /* upper threshold */
+       u_int32_t               l_threshold;    /* lower threshold */
+
+       u_int32_t               activeconns;    /* active connections */
+       u_int32_t               inactconns;     /* inactive connections */
+       u_int32_t               persistconns;   /* persistent connections */
+
+       /* statistics */
+       struct ip_vs_stats_user stats;

You might also have a hole before stats since it has 8b alignment.
I'd suggest to use pahole to figure out which structs need to be
restructured.

...

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