BUG/MAJOR: stats-file: ensure shm_stats_file_object struct mapping consistency (2nd...

This is a second attempt at fixing issues on 32bits systems which would
trigger the following BUG_ON() statement:

 FATAL: bug condition "sizeof(struct shm_stats_file_object) != 544" matched at src/stats-file.c:825 shm_stats_file_object struct size changed, is is part of the exported API: ensure all precautions were taken (ie: shm_stats_file version change) before adjusting this

This is a drop-in replacement for d30b88a6c + 4693ee0ff, as suggested by
Willy.

Indeed, on supported platforms unsigned int can be assumed to be 4 bytes
long, and long can be assumed to be 8 bytes long. As such, the previous
attempt was overkill and added unecessary maintenance complexity which
could result in bugs if not used properly. Moreover, it would only
partially solve the issue, since on little endian vs big endian
architectures, the provisioned memory areas (originating from the same
shm stats file) could be read differently by the host.

Instead we fix the aligments issues, and this alone helps to ensure
struct memory consistency on 64 vs 32bits platforms. It was tested
on both i386 and i586.

last_change and last_sess counters are now stored as unsigned int, as
it helped to fix the alignment issues and they were found to be used
as 32bits integers anyway.

Thanks to Willy for problem analysis and the patch proposal.

No backport needed.

diff --git a/include/haproxy/counters-t.h b/include/haproxy/counters-t.h
index 13dd8f88a39d084301ab23de1df190b67e920386..d98990f13dabf2ae4cc24dafb0bc20f9841f593b 100644 (file)
--- a/include/haproxy/counters-t.h
+++ b/include/haproxy/counters-t.h
@@ -36,11 +36,11 @@
 
 /* /!\ any change performed here will impact shm-stats-file mapping because the
  * struct is embedded in shm_stats_file_object struct, so proceed with caution
- * and change shm stats file version if needed
+ * and change shm stats file version if needed. Also please always keep this
+ * struct 64b-aligned.
  */
 #define COUNTERS_SHARED_TG                                                           \
        struct {                                                                     \
-               unsigned long last_state_change;        /* last time, when the state was changed */\
                long long srv_aborts;                   /* aborted responses during DATA phase caused by the server */\
                long long cli_aborts;                   /* aborted responses during DATA phase caused by the client */\
                long long internal_errors;              /* internal processing errors */\
@@ -54,7 +54,9 @@
                long long comp_in[2];                   /* input bytes fed to the compressor */\
                long long comp_out[2];                  /* output bytes emitted by the compressor */\
                long long comp_byp[2];                  /* input bytes that bypassed the compressor (cpu/ram/bw limitation) */\
-               struct freq_ctr sess_per_sec;           /* sessions per second on this server */\
+               struct freq_ctr sess_per_sec;           /* sessions per second on this server (3x32b) */\
+               unsigned int last_state_change;         /* last time, when the state was changed (32b) */\
+               /* we're still 64b-aligned here */ \
        }
 
 // for convenience (generic pointer)
@@ -93,7 +95,7 @@ struct fe_counters_shared_tg {
        } p;                                    /* protocol-specific stats */
 
        long long failed_req;                   /* failed requests (eg: invalid or timeout) */
-};
+} ALIGNED(8);
 
 struct fe_counters_shared {
        COUNTERS_SHARED;
@@ -120,7 +122,8 @@ struct fe_counters {
 
 /* /!\ any change performed here will impact shm-stats-file mapping because the
  * struct is embedded in shm_stats_file_object struct, so proceed with caution
- * and change shm stats file version if needed
+ * and change shm stats file version if needed. Pay attention to keeping the
+ * struct 64b-aligned.
  */
 struct be_counters_shared_tg {
        COUNTERS_SHARED_TG;
@@ -129,7 +132,6 @@ struct be_counters_shared_tg {
 
        long long connect;                      /* number of connection establishment attempts */
        long long reuse;                        /* number of connection reuses */
-       unsigned long last_sess;                /* last session time */
 
        long long failed_checks, failed_hana;   /* failed health checks and health analyses for servers */
        long long down_trans;                   /* up->down transitions */
@@ -150,7 +152,9 @@ struct be_counters_shared_tg {
        long long retries;                      /* retried and redispatched connections (BE only) */
        long long failed_resp;                  /* failed responses (BE only) */
        long long failed_conns;                 /* failed connect() attempts (BE only) */
-};
+       unsigned int last_sess;                 /* last session time */
+       /* 32-bit hole here */
+} ALIGNED(8);
 
 struct be_counters_shared {
        COUNTERS_SHARED;

diff --git a/src/stats-file.c b/src/stats-file.c
index 7aefb2f6b3f20f5a53f70103d1e7ab02edb11272..5ccf0c4dadd86937e89a6f4c530911014fa2b6be 100644 (file)
--- a/src/stats-file.c
+++ b/src/stats-file.c
@@ -821,7 +821,7 @@ int shm_stats_file_prepare(void)
        BUG_ON(sizeof(struct shm_stats_file_hdr) != 672, "shm_stats_file_hdr struct size changed, "
               "it is part of the exported API: ensure all precautions were taken (ie: shm_stats_file "
               "version change) before adjusting this");
-       BUG_ON(sizeof(struct shm_stats_file_object) != 544, "shm_stats_file_object struct size changed, "
+       BUG_ON(sizeof(struct shm_stats_file_object) != 536, "shm_stats_file_object struct size changed, "
               "it is part of the exported API: ensure all precautions were taken (ie: shm_stats_file "
               "version change) before adjusting this");