[Xen-devel] 答复: [PATCH] ACPI/ERST: fix signed/unsigned type conflicts

["刘劲松(凯耳)" <jinsong.liu@xxxxxxxxxxxxxxx>]

Fine to me, except I'm not sure the BUILD_BUG_ON at init_constructors() of
lib.c

Thanks,
Jinsong

-----邮件原件-----
发件人: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
发送时间: 2014年4月14日 18:19
收件人: xen-devel
抄送: 刘劲松(凯耳); Christoph Egger; Ian Campbell; Stefano Stabellini; Keir
Fraser; Tim Deegan
主题: [PATCH] ACPI/ERST: fix signed/unsigned type conflicts

Error checks exist in the respective code path that expect negative values
to indicate errors, yet negative values can't be communicated through
size_t. Thus ssize_t needs to be introduced (also on ARM for consistency,
even if the code in question isn't currently being used on there).

The bug is theoretical only in so far as all the involved code is
effectively dead. Reflect this by excluding that code from non-debug builds.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -89,10 +89,12 @@ int apei_write_mce(struct mce *m)
        return erst_write(&rcd.hdr);
 }
 
-size_t apei_read_mce(struct mce *m, u64 *record_id)
+#ifndef NDEBUG /* currently dead code */
+
+ssize_t apei_read_mce(struct mce *m, u64 *record_id)
 {
        struct cper_mce_record rcd;
-       size_t len;
+       ssize_t len;
 
        if (!m || !record_id)
                return -EINVAL;
@@ -115,12 +117,14 @@ size_t apei_read_mce(struct mce *m, u64  }
 
 /* Check whether there is record in ERST */ -int apei_check_mce(void)
+bool_t apei_check_mce(void)
 {
-       return erst_get_record_count();
+       return erst_get_record_count() > 0;
 }
 
 int apei_clear_mce(u64 record_id)
 {
        return erst_clear(record_id);
 }
+
+#endif /* currently dead code */
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -487,6 +487,9 @@ void __init init_constructors(void)
     const ctor_func_t *f;
     for ( f = __ctors_start; f < __ctors_end; ++f )
         (*f)();
+
+    /* Putting this here seems as good (or bad) as any other place. */
+    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
 }
 
 /*
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -383,21 +383,24 @@ static int erst_get_erange(struct erst_e
        return 0;
 }
 
-static size_t __erst_get_record_count(void)
+static ssize_t __erst_get_record_count(void)
 {
        struct apei_exec_context ctx;
        int rc;
+       u64 output;
+       ssize_t count;
 
        erst_exec_ctx_init(&ctx);
        rc = apei_exec_run(&ctx, ACPI_ERST_GET_RECORD_COUNT);
        if (rc)
                return rc;
-       return apei_exec_ctx_get_output(&ctx);
+       count = output = apei_exec_ctx_get_output(&ctx);
+       return count >= 0 && count == output ? count : -ERANGE;
 }
 
-size_t erst_get_record_count(void)
+ssize_t erst_get_record_count(void)
 {
-       size_t count;
+       ssize_t count;
        unsigned long flags;
 
        if (!erst_enabled)
@@ -483,6 +486,8 @@ static int __erst_write_to_storage(u64 o
        return erst_errno(val);
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_from_storage(u64 record_id, u64 offset)  {
        struct apei_exec_context ctx;
@@ -565,6 +570,8 @@ static int __erst_clear_from_storage(u64
        return erst_errno(val);
 }
 
+#endif /* currently dead code */
+
 /* NVRAM ERST Error Log Address Range is not supported yet */  static int
__erst_write_to_nvram(const struct cper_record_header *record)  { @@ -572,6
+579,8 @@ static int __erst_write_to_nvram(const s
        return -ENOSYS;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)  {
        printk(KERN_WARNING
@@ -586,6 +595,8 @@ static int __erst_clear_from_nvram(u64 r
        return -ENOSYS;
 }
 
+#endif /* currently dead code */
+
 int erst_write(const struct cper_record_header *record)  {
        int rc;
@@ -625,6 +636,8 @@ int erst_write(const struct cper_record_
        return rc;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange(u64 record_id, u64 *offset)  {
        int rc;
@@ -641,22 +654,24 @@ static int __erst_read_to_erange(u64 rec
        return 0;
 }
 
-static size_t __erst_read(u64 record_id, struct cper_record_header *record,
+static ssize_t __erst_read(u64 record_id, struct cper_record_header 
+*record,
                           size_t buflen)
 {
        int rc;
-       u64 offset, len = 0;
+       u64 offset;
+       ssize_t len;
        struct cper_record_header *rcd_tmp;
 
        rc = __erst_read_to_erange(record_id, &offset);
        if (rc)
                return rc;
        rcd_tmp = erst_erange.vaddr + offset;
+       if (rcd_tmp->record_length > buflen)
+               return -ENOBUFS;
        len = rcd_tmp->record_length;
-       if (len <= buflen)
-               memcpy(record, rcd_tmp, len);
+       memcpy(record, rcd_tmp, len);
 
-       return len;
+       return len >= 0 ? len : -ERANGE;
 }
 
 /*
@@ -664,10 +679,10 @@ static size_t __erst_read(u64 record_id,
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
                  size_t buflen)
 {
-       size_t len;
+       ssize_t len;
        unsigned long flags;
 
        if (!erst_enabled)
@@ -685,10 +700,10 @@ size_t erst_read(u64 record_id, struct c
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read_next(struct cper_record_header *record, size_t buflen)
+ssize_t erst_read_next(struct cper_record_header *record, size_t 
+buflen)
 {
        int rc;
-       size_t len;
+       ssize_t len;
        unsigned long flags;
        u64 record_id;
 
@@ -731,6 +746,8 @@ int erst_clear(u64 record_id)
        return rc;
 }
 
+#endif /* currently dead code */
+
 static int __init erst_check_table(struct acpi_table_erst *erst_tab)  {
        if (erst_tab->header.length < sizeof(*erst_tab))
--- a/xen/include/acpi/apei.h
+++ b/xen/include/acpi/apei.h
@@ -13,11 +13,11 @@
 #define FIX_APEI_RANGE_MAX 64
 
 int erst_write(const struct cper_record_header *record); -size_t
erst_get_record_count(void);
+ssize_t erst_get_record_count(void);
 int erst_get_next_record_id(u64 *record_id); -size_t erst_read(u64
record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
                  size_t buflen);
-size_t erst_read_next(struct cper_record_header *record, size_t buflen);
+ssize_t erst_read_next(struct cper_record_header *record, size_t 
+buflen);
 int erst_clear(u64 record_id);
 
 void __iomem *apei_pre_map(paddr_t paddr, unsigned long size);
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -56,6 +56,7 @@ typedef u64 register_t;  #endif
 
 typedef unsigned long size_t;
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)
--- a/xen/include/asm-x86/types.h
+++ b/xen/include/asm-x86/types.h
@@ -39,6 +39,7 @@ typedef __SIZE_TYPE__ size_t;  #else  typedef unsigned
long size_t;  #endif
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel