-Wattribute-warning in drivers/crypto/hisilicon/sgl.c

[nathanchance]

After commit f7cd05c ("fortify: Use __builtin_dynamic_object_size() when available") in -next, I see the following warning when building at least ARCH=arm64 allmodconfig;

In file included from ../drivers/crypto/hisilicon/sgl.c:4:
In file included from ../include/linux/dma-mapping.h:6:
In file included from ../include/linux/string.h:253:
../include/linux/fortify-string.h:430:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                        __write_overflow_field(p_size_field, size);
                        ^
1 error generated.

I think this is a legitimate complaint? The memset() in hisi_acc_create_sgl_pool() appears to want to zero a single element of the mem_block array in struct hisi_acc_sgl_pool (block + j) but it uses the entire size of the array (sizeof(*block)) versus one element. Unless I am missing something, why not just zero the whole array after the loop? This diff clears up the warning:

diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
index 2b6f2281cfd6..83657463f949 100644
--- a/drivers/crypto/hisilicon/sgl.c
+++ b/drivers/crypto/hisilicon/sgl.c
@@ -121,11 +121,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
        return pool;

 err_free_mem:
-       for (j = 0; j < i; j++) {
+       for (j = 0; j < i; j++)
                dma_free_coherent(dev, block_size, block[j].sgl,
                                  block[j].sgl_dma);
-               memset(block + j, 0, sizeof(*block));
-       }
+       memset(block, 0, sizeof(*block));
        kfree(pool);
        return ERR_PTR(-ENOMEM);
 }

Otherwise, we could just switch this to a struct_group() as the warning suggests? I did not mess around with what that would look like though or if it is feasible. cc @kees, just to make sure I did not miss something.

[nathanchance]

Obviously, the suggestion should be:

diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
index 2b6f2281cfd6..5068797b8b02 100644
--- a/drivers/crypto/hisilicon/sgl.c
+++ b/drivers/crypto/hisilicon/sgl.c
@@ -121,11 +121,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
        return pool;

 err_free_mem:
-       for (j = 0; j < i; j++) {
+       for (j = 0; j < i; j++)
                dma_free_coherent(dev, block_size, block[j].sgl,
                                  block[j].sgl_dma);
-               memset(block + j, 0, sizeof(*block));
-       }
+       memset(block, 0, sizeof(block));
        kfree(pool);
        return ERR_PTR(-ENOMEM);
 }

and my understanding of the problem was incorrect (good ol' sizeof(*a) vs. sizeof(a)...) but I still think that would probably be the simplest solution, as I do not see a reason for block not to be completely zeroed out in this path. I am not sure struct_group() will work with the array.

[kees]

sizeof(*block) is sizeof(struct mem_block). sizeof(block) is sizeof(void *), so, I think the correct fix here is:

diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
index 2b6f2281cfd6..97a64ccfe41e 100644
--- a/drivers/crypto/hisilicon/sgl.c
+++ b/drivers/crypto/hisilicon/sgl.c
@@ -124,7 +124,7 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
        for (j = 0; j < i; j++) {
                dma_free_coherent(dev, block_size, block[j].sgl,
                                  block[j].sgl_dma);
-               memset(block + j, 0, sizeof(*block));
+               memset(&block[j], 0, sizeof(*block));
        }
        kfree(pool);
        return ERR_PTR(-ENOMEM);

But that doesn't seem to fix it. Hmm.

[nathanchance]

But that doesn't seem to fix it. Hmm.

Right, I should have mentioned at some point that I did try that and it did not resolve it for me. Perhaps the flexible array member in struct hisi_acc_hw_sgl is messing with this calculation?

[kees]

sizeof(block) == 8
sizeof(*block) == 24
sizeof(&block[0]) = 8
sizeof(pool->mem_block) == 120

So, this works:

-               memset(block + j, 0, sizeof(*block));
        }
+       memset(pool->mem_block, 0, sizeof(pool->mem_block));

But in trying to figure this out, it seems like something is very weird about dealing with arrays:

__builtin_object_size(block, 1) == 144
__builtin_object_size(&block[0], 1) == 144
__builtin_object_size(pool->mem_block, 1) == 120

Oddly, this is the same for `__builtin_dynamic_object_size(..., 1)``` too.

[kees]

It seems like Clang lost track of the object size, and "mode 1" reverted to "mode 0"?

struct hisi_acc_sgl_pool {
        struct mem_block           mem_block[5];         /*     0   120 */
        /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
        u32                        sgl_num_per_block;    /*   120     4 */
        u32                        block_num;            /*   124     4 */
        /* --- cacheline 2 boundary (128 bytes) --- */
        u32                        count;                /*   128     4 */
        u32                        sge_nr;               /*   132     4 */
        size_t                     sgl_size;             /*   136     8 */

        /* size: 144, cachelines: 3, members: 6 */
        /* last cacheline: 16 bytes */
};

__builtin_object_size(block, 1) is reporting the size of the entire struct. :( :(

[kees]

I've opened an LLVM bug, possibly releated:
llvm/llvm-project#59850

[kees]

https://lore.kernel.org/lkml/20230106041945.never.831-kees@kernel.org

[nathanchance]

This was worked around in 6.3: https://git.kernel.org/linus/aa85923a954e7704bc9d3847dabeb8540aa98d13

It sounds like this is currently a known limitation of __bos() (llvm/llvm-project#55742), so I am going to close this up for now.