[bug report] iio: pressure: bmp280: drop sensor_data array

[bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter]

Hello David Lechner,

Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
from Apr 22, 2025 (linux-next), leads to the following Smatch static
checker warning:

	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')

drivers/iio/pressure/bmp280-core.c
    1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
    1226 {
    1227         struct iio_poll_func *pf = p;
    1228         struct iio_dev *indio_dev = pf->indio_dev;
    1229         struct bmp280_data *data = iio_priv(indio_dev);
    1230         u32 adc_temp, adc_press, adc_humidity;
    1231         s32 t_fine;
    1232         struct {
    1233                 u32 comp_press;
    1234                 s32 comp_temp;
    1235                 u32 comp_humidity;
    1236                 aligned_s64 timestamp;

There is a 4 byte hole between comp_humidity and timestamp.

    1237         } buffer;
    1238         int ret;
    1239 
    1240         guard(mutex)(&data->lock);
    1241 
    1242         /* Burst read data registers */
    1243         ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
    1244                                data->buf, BME280_BURST_READ_BYTES);
    1245         if (ret) {
    1246                 dev_err(data->dev, "failed to burst read sensor data\n");
    1247                 goto out;
    1248         }
    1249 
    1250         /* Temperature calculations */
    1251         adc_temp = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[3]));
    1252         if (adc_temp == BMP280_TEMP_SKIPPED) {
    1253                 dev_err(data->dev, "reading temperature skipped\n");
    1254                 goto out;
    1255         }
    1256 
    1257         buffer.comp_temp = bmp280_compensate_temp(data, adc_temp);
    1258 
    1259         /* Pressure calculations */
    1260         adc_press = FIELD_GET(BMP280_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[0]));
    1261         if (adc_press == BMP280_PRESS_SKIPPED) {
    1262                 dev_err(data->dev, "reading pressure skipped\n");
    1263                 goto out;
    1264         }
    1265 
    1266         t_fine = bmp280_calc_t_fine(data, adc_temp);
    1267         buffer.comp_press = bmp280_compensate_press(data, adc_press, t_fine);
    1268 
    1269         /* Humidity calculations */
    1270         adc_humidity = get_unaligned_be16(&data->buf[6]);
    1271 
    1272         if (adc_humidity == BMP280_HUMIDITY_SKIPPED) {
    1273                 dev_err(data->dev, "reading humidity skipped\n");
    1274                 goto out;
    1275         }
    1276 
    1277         buffer.comp_humidity = bme280_compensate_humidity(data, adc_humidity,
    1278                                                           t_fine);
    1279 
--> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
                                                        ^^^^^^^^^^^^^^^^^^^^^^^
So I believe it leads to an information leaks here.

    1281                                     iio_get_time_ns(indio_dev));
    1282 
    1283 out:
    1284         iio_trigger_notify_done(indio_dev->trig);
    1285 
    1286         return IRQ_HANDLED;
    1287 }

regards,
dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[David Lechner]

On 5/6/25 7:32 AM, Dan Carpenter wrote:
> Hello David Lechner,
> 
> Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> from Apr 22, 2025 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> 
> drivers/iio/pressure/bmp280-core.c
>     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
>     1226 {
>     1227         struct iio_poll_func *pf = p;
>     1228         struct iio_dev *indio_dev = pf->indio_dev;
>     1229         struct bmp280_data *data = iio_priv(indio_dev);
>     1230         u32 adc_temp, adc_press, adc_humidity;
>     1231         s32 t_fine;
>     1232         struct {
>     1233                 u32 comp_press;
>     1234                 s32 comp_temp;
>     1235                 u32 comp_humidity;
>     1236                 aligned_s64 timestamp;
> 
> There is a 4 byte hole between comp_humidity and timestamp.

Yes, this was the intention of this patch.

> 
>     1237         } buffer;
>     1238         int ret;
>     1239 

...

>     1279 
> --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
>                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> So I believe it leads to an information leaks here.

Aha, so we should e.g. do memset() to fill the hole first.

> 
>     1281                                     iio_get_time_ns(indio_dev));
>     1282 
>     1283 out:
>     1284         iio_trigger_notify_done(indio_dev->trig);
>     1285 
>     1286         return IRQ_HANDLED;
>     1287 }
> 
> regards,
> dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter]

On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
> On 5/6/25 7:32 AM, Dan Carpenter wrote:
> > Hello David Lechner,
> > 
> > Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> > from Apr 22, 2025 (linux-next), leads to the following Smatch static
> > checker warning:
> > 
> > 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> > 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> > 
> > drivers/iio/pressure/bmp280-core.c
> >     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
> >     1226 {
> >     1227         struct iio_poll_func *pf = p;
> >     1228         struct iio_dev *indio_dev = pf->indio_dev;
> >     1229         struct bmp280_data *data = iio_priv(indio_dev);
> >     1230         u32 adc_temp, adc_press, adc_humidity;
> >     1231         s32 t_fine;
> >     1232         struct {
> >     1233                 u32 comp_press;
> >     1234                 s32 comp_temp;
> >     1235                 u32 comp_humidity;
> >     1236                 aligned_s64 timestamp;
> > 
> > There is a 4 byte hole between comp_humidity and timestamp.
> 
> Yes, this was the intention of this patch.
> 
> > 
> >     1237         } buffer;
> >     1238         int ret;
> >     1239 
> 
> ...
> 
> >     1279 
> > --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
> >                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> > So I believe it leads to an information leaks here.
> 
> Aha, so we should e.g. do memset() to fill the hole first.
> 

That works, or you could initialize it with = { }.

regards,
dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Jonathan Cameron]

On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
>> On 5/6/25 7:32 AM, Dan Carpenter wrote:
>> > Hello David Lechner,
>> > 
>> > Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
>> > from Apr 22, 2025 (linux-next), leads to the following Smatch static
>> > checker warning:
>> > 
>> > 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
>> > 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
>> > 
>> > drivers/iio/pressure/bmp280-core.c
>> >     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
>> >     1226 {
>> >     1227         struct iio_poll_func *pf = p;
>> >     1228         struct iio_dev *indio_dev = pf->indio_dev;
>> >     1229         struct bmp280_data *data = iio_priv(indio_dev);
>> >     1230         u32 adc_temp, adc_press, adc_humidity;
>> >     1231         s32 t_fine;
>> >     1232         struct {
>> >     1233                 u32 comp_press;
>> >     1234                 s32 comp_temp;
>> >     1235                 u32 comp_humidity;
>> >     1236                 aligned_s64 timestamp;
>> > 
>> > There is a 4 byte hole between comp_humidity and timestamp.
>> 
>> Yes, this was the intention of this patch.
>> 
>> > 
>> >     1237         } buffer;
>> >     1238         int ret;
>> >     1239 
>> 
>> ...
>> 
>> >     1279 
>> > --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
>> >                                                         ^^^^^^^^^^^^^^^^^^^^^^^
>> > So I believe it leads to an information leaks here.
>> 
>> Aha, so we should e.g. do memset() to fill the hole first.
>> 
>
>That works, or you could initialize it with = { }.

I tried to track this down the other day.
What are compilers guaranteeing around
that vs { 0 }  and holes?  The c spec has only recently standardised on { }.

I'd love to stop using memset for these.


J
>
>regards,
>dan carpenter
>
>

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter]

On Wed, May 07, 2025 at 07:35:27AM +0100, Jonathan Cameron wrote:
> 
> 
> On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
> >> On 5/6/25 7:32 AM, Dan Carpenter wrote:
> >> > Hello David Lechner,
> >> > 
> >> > Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> >> > from Apr 22, 2025 (linux-next), leads to the following Smatch static
> >> > checker warning:
> >> > 
> >> > 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> >> > 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> >> > 
> >> > drivers/iio/pressure/bmp280-core.c
> >> >     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
> >> >     1226 {
> >> >     1227         struct iio_poll_func *pf = p;
> >> >     1228         struct iio_dev *indio_dev = pf->indio_dev;
> >> >     1229         struct bmp280_data *data = iio_priv(indio_dev);
> >> >     1230         u32 adc_temp, adc_press, adc_humidity;
> >> >     1231         s32 t_fine;
> >> >     1232         struct {
> >> >     1233                 u32 comp_press;
> >> >     1234                 s32 comp_temp;
> >> >     1235                 u32 comp_humidity;
> >> >     1236                 aligned_s64 timestamp;
> >> > 
> >> > There is a 4 byte hole between comp_humidity and timestamp.
> >> 
> >> Yes, this was the intention of this patch.
> >> 
> >> > 
> >> >     1237         } buffer;
> >> >     1238         int ret;
> >> >     1239 
> >> 
> >> ...
> >> 
> >> >     1279 
> >> > --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
> >> >                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> >> > So I believe it leads to an information leaks here.
> >> 
> >> Aha, so we should e.g. do memset() to fill the hole first.
> >> 
> >
> >That works, or you could initialize it with = { }.
> 
> I tried to track this down the other day.
> What are compilers guaranteeing around
> that vs { 0 }  and holes?  The c spec has only recently standardised on { }.
> 
> I'd love to stop using memset for these.

The rule in the C standard is that if the initializer sets every struct
member then it will NOT zero out struct holes.  But if there are any
unset struct members then the holes are zeroed.  So = { } will always
work.  You really have to try hard to invent a scenario where = { 0 }
won't fill the struct holes (a one member struct with a weird alignment).

regards,
dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[David Lechner]

On 5/7/25 2:41 AM, Dan Carpenter wrote:
> On Wed, May 07, 2025 at 07:35:27AM +0100, Jonathan Cameron wrote:
>>
>>
>> On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>> On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
>>>> On 5/6/25 7:32 AM, Dan Carpenter wrote:
>>>>> Hello David Lechner,
>>>>>
>>>>> Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
>>>>> from Apr 22, 2025 (linux-next), leads to the following Smatch static
>>>>> checker warning:
>>>>>
>>>>> 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
>>>>> 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
>>>>>
>>>>> drivers/iio/pressure/bmp280-core.c
>>>>>     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
>>>>>     1226 {
>>>>>     1227         struct iio_poll_func *pf = p;
>>>>>     1228         struct iio_dev *indio_dev = pf->indio_dev;
>>>>>     1229         struct bmp280_data *data = iio_priv(indio_dev);
>>>>>     1230         u32 adc_temp, adc_press, adc_humidity;
>>>>>     1231         s32 t_fine;
>>>>>     1232         struct {
>>>>>     1233                 u32 comp_press;
>>>>>     1234                 s32 comp_temp;
>>>>>     1235                 u32 comp_humidity;
>>>>>     1236                 aligned_s64 timestamp;
>>>>>
>>>>> There is a 4 byte hole between comp_humidity and timestamp.
>>>>
>>>> Yes, this was the intention of this patch.
>>>>
>>>>>
>>>>>     1237         } buffer;
>>>>>     1238         int ret;
>>>>>     1239 
>>>>
>>>> ...
>>>>
>>>>>     1279 
>>>>> --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
>>>>>                                                         ^^^^^^^^^^^^^^^^^^^^^^^
>>>>> So I believe it leads to an information leaks here.
>>>>
>>>> Aha, so we should e.g. do memset() to fill the hole first.
>>>>
>>>
>>> That works, or you could initialize it with = { }.
>>
>> I tried to track this down the other day.
>> What are compilers guaranteeing around
>> that vs { 0 }  and holes?  The c spec has only recently standardised on { }.
>>
>> I'd love to stop using memset for these.
> 
> The rule in the C standard is that if the initializer sets every struct
> member then it will NOT zero out struct holes.  But if there are any
> unset struct members then the holes are zeroed.  So = { } will always
> work.  You really have to try hard to invent a scenario where = { 0 }
> won't fill the struct holes (a one member struct with a weird alignment).
> 
> regards,
> dan carpenter
> 

I was curious about this too and came across a blog post [1] that claims that
with clang compiler and certain optimization levels, { } and { 0 } still aren't
good enough, which is why I went with the memset().

[1]: https://interrupt.memfault.com/blog/c-struct-padding-initialization

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter]

Let me add Kees to the CC list because he'll want to know this as well.

On Wed, May 07, 2025 at 08:33:07AM -0500, David Lechner wrote:
> On 5/7/25 2:41 AM, Dan Carpenter wrote:
> > On Wed, May 07, 2025 at 07:35:27AM +0100, Jonathan Cameron wrote:
> >>
> >>
> >> On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >>> On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
> >>>> On 5/6/25 7:32 AM, Dan Carpenter wrote:
> >>>>> Hello David Lechner,
> >>>>>
> >>>>> Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> >>>>> from Apr 22, 2025 (linux-next), leads to the following Smatch static
> >>>>> checker warning:
> >>>>>
> >>>>> 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> >>>>> 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> >>>>>
> >>>>> drivers/iio/pressure/bmp280-core.c
> >>>>>     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
> >>>>>     1226 {
> >>>>>     1227         struct iio_poll_func *pf = p;
> >>>>>     1228         struct iio_dev *indio_dev = pf->indio_dev;
> >>>>>     1229         struct bmp280_data *data = iio_priv(indio_dev);
> >>>>>     1230         u32 adc_temp, adc_press, adc_humidity;
> >>>>>     1231         s32 t_fine;
> >>>>>     1232         struct {
> >>>>>     1233                 u32 comp_press;
> >>>>>     1234                 s32 comp_temp;
> >>>>>     1235                 u32 comp_humidity;
> >>>>>     1236                 aligned_s64 timestamp;
> >>>>>
> >>>>> There is a 4 byte hole between comp_humidity and timestamp.
> >>>>
> >>>> Yes, this was the intention of this patch.
> >>>>
> >>>>>
> >>>>>     1237         } buffer;
> >>>>>     1238         int ret;
> >>>>>     1239 
> >>>>
> >>>> ...
> >>>>
> >>>>>     1279 
> >>>>> --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
> >>>>>                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> >>>>> So I believe it leads to an information leaks here.
> >>>>
> >>>> Aha, so we should e.g. do memset() to fill the hole first.
> >>>>
> >>>
> >>> That works, or you could initialize it with = { }.
> >>
> >> I tried to track this down the other day.
> >> What are compilers guaranteeing around
> >> that vs { 0 }  and holes?  The c spec has only recently standardised on { }.
> >>
> >> I'd love to stop using memset for these.
> > 
> > The rule in the C standard is that if the initializer sets every struct
> > member then it will NOT zero out struct holes.  But if there are any
> > unset struct members then the holes are zeroed.  So = { } will always
> > work.  You really have to try hard to invent a scenario where = { 0 }
> > won't fill the struct holes (a one member struct with a weird alignment).
> > 
> > regards,
> > dan carpenter
> > 
> 
> I was curious about this too and came across a blog post [1] that claims that
> with clang compiler and certain optimization levels, { } and { 0 } still aren't
> good enough, which is why I went with the memset().
> 
> [1]: https://interrupt.memfault.com/blog/c-struct-padding-initialization

Huh...

"Strategy 2, explicitly setting each struct member".  This isn't
"supposed" to initialize struct holes according to the C current
standard.

It's discouraging that = { 0 } and { } don't work.  -O1 is not supported
in the kernel so that's not an emergency.  I don't know about -Os?

regards,
dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter]

No, I looked at test code from Noah Pendleton it has a mistake.  He's
testing assignments, not initialization.  It's a different thing.

I also looked up the relevant portion from the C11 standard (6.7.9)

  — if it is an aggregate, every member is initialized (recursively)
    according to these rules, and any padding is initialized to zero
    bits;

Which sounds like padding is always zeroed, but the context of this
quote is that we didn't fully set every struct member.  We drop
support for compiler that are over 10 years old so C11 covers
everything we support.

Here is the test code that Noah was using.

https://github.com/memfault/interrupt/blob/master/example/c-struct-padding-initialization/example.c

	struct foo foo;

	// 3. use { 0 } zero-initializer
	memset(&foo, 0xa5, sizeof(foo));
	foo = (struct foo){0};

There isn't an initializer on the foo struct.  Then he does a memset()
and assigns a struct to foo.  Assigning one struct to the other is a
different section of the C standard.

I created my own test:

struct foo {
	unsigned int i;
	unsigned char b;
	// 3 bytes of padding inserted here, UNLESS -fpack-struct is used!
};

static void print_struct(void *buffer, int size)
{
	unsigned char *p = buffer;
	int i;

	for (i = 0; i < sizeof(struct foo); i++) {
		printf("0x%x ", p[i]);
	}
	printf("\n");
}

int main(int argc, char *argv[])
{
	struct foo one = { 0, 0 };
	struct foo two = { 0 };
	struct foo three = { };

	print_struct(&one, sizeof(struct foo));
	print_struct(&two, sizeof(struct foo));
	print_struct(&three, sizeof(struct foo));

	return 0;
}

GCC does not initialize "one" because it's fully defined.  Clang does
initialize it (because Clang goes above and beyond for security).  The
rest are zeroed as expected.

regards,
dan carpenter

Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Kees Cook]

On Fri, May 09, 2025 at 08:49:30AM +0300, Dan Carpenter wrote:
> Let me add Kees to the CC list because he'll want to know this as well.
> 
> On Wed, May 07, 2025 at 08:33:07AM -0500, David Lechner wrote:
> > On 5/7/25 2:41 AM, Dan Carpenter wrote:
> > > On Wed, May 07, 2025 at 07:35:27AM +0100, Jonathan Cameron wrote:
> > >>
> > >>
> > >> On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >>> On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
> > >>>> On 5/6/25 7:32 AM, Dan Carpenter wrote:
> > >>>>> Hello David Lechner,
> > >>>>>
> > >>>>> Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> > >>>>> from Apr 22, 2025 (linux-next), leads to the following Smatch static
> > >>>>> checker warning:
> > >>>>>
> > >>>>> 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> > >>>>> 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> > >>>>>
> > >>>>> drivers/iio/pressure/bmp280-core.c
> > >>>>>     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
> > >>>>>     1226 {
> > >>>>>     1227         struct iio_poll_func *pf = p;
> > >>>>>     1228         struct iio_dev *indio_dev = pf->indio_dev;
> > >>>>>     1229         struct bmp280_data *data = iio_priv(indio_dev);
> > >>>>>     1230         u32 adc_temp, adc_press, adc_humidity;
> > >>>>>     1231         s32 t_fine;
> > >>>>>     1232         struct {
> > >>>>>     1233                 u32 comp_press;
> > >>>>>     1234                 s32 comp_temp;
> > >>>>>     1235                 u32 comp_humidity;
> > >>>>>     1236                 aligned_s64 timestamp;
> > >>>>>
> > >>>>> There is a 4 byte hole between comp_humidity and timestamp.
> > >>>>
> > >>>> Yes, this was the intention of this patch.
> > >>>>
> > >>>>>
> > >>>>>     1237         } buffer;
> > >>>>>     1238         int ret;
> > >>>>>     1239 
> > >>>>
> > >>>> ...
> > >>>>
> > >>>>>     1279 
> > >>>>> --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
> > >>>>>                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> > >>>>> So I believe it leads to an information leaks here.
> > >>>>
> > >>>> Aha, so we should e.g. do memset() to fill the hole first.
> > >>>>
> > >>>
> > >>> That works, or you could initialize it with = { }.
> > >>
> > >> I tried to track this down the other day.
> > >> What are compilers guaranteeing around
> > >> that vs { 0 }  and holes?  The c spec has only recently standardised on { }.
> > >>
> > >> I'd love to stop using memset for these.
> > > 
> > > The rule in the C standard is that if the initializer sets every struct
> > > member then it will NOT zero out struct holes.  But if there are any
> > > unset struct members then the holes are zeroed.  So = { } will always
> > > work.  You really have to try hard to invent a scenario where = { 0 }
> > > won't fill the struct holes (a one member struct with a weird alignment).
> > > 
> > > regards,
> > > dan carpenter
> > > 
> > 
> > I was curious about this too and came across a blog post [1] that claims that
> > with clang compiler and certain optimization levels, { } and { 0 } still aren't
> > good enough, which is why I went with the memset().
> > 
> > [1]: https://interrupt.memfault.com/blog/c-struct-padding-initialization
> 
> Huh...
> 
> "Strategy 2, explicitly setting each struct member".  This isn't
> "supposed" to initialize struct holes according to the C current
> standard.
> 
> It's discouraging that = { 0 } and { } don't work.  -O1 is not supported
> in the kernel so that's not an emergency.  I don't know about -Os?

tl;dr: Use { }.

I didn't think -Os was supported either?

The stackinit KUnit tests will do all these checks, FWIW. It's a little
difficult to decode the results, but this is consistent with the
findings. Looking at a similar case to above, the "small_hole" struct
test:

struct test_small_hole {
        size_t one;
        char two;
        /* 3 byte padding hole here. */
        int three;
        unsigned long four;
};

Tested with various initializers:

#define INIT_STRUCT_none(var_type)      /**/
#define INIT_STRUCT_zero(var_type)      = { }
#define INIT_STRUCT_old_zero(var_type)  = { 0 }

#define __static_partial                { .two = 0, }
#define __static_all                    { .one = 0, \
                                          .two = 0, \
                                          .three = 0, \
                                          .four = 0, \
                                        }
#define __dynamic_partial               { .two = arg->two, }
#define __dynamic_all                   { .one = arg->one, \
                                          .two = arg->two, \
                                          .three = arg->three, \
                                          .four = arg->four, \
                                        }
#define __runtime_partial               var.two = 0
#define __runtime_all                   var.one = 0; \
                                        var.two = 0; \
                                        var.three = 0; \
                                        var.four = 0


We can see a default build (-O2), "PASSED" means all zero, and "SKIPPED"
means "not all zero, but we expected that given the Kconfig involved":

(Running "./tools/testing/kunit/kunit.py run stackinit")

[09:44:47] [PASSED] test_small_hole_zero
[09:44:47] [PASSED] test_small_hole_old_zero
[09:44:47] [PASSED] test_small_hole_dynamic_partial
[09:44:47] [PASSED] test_small_hole_assigned_dynamic_partial
[09:44:47] [PASSED] test_small_hole_static_partial
[09:44:47] [PASSED] test_small_hole_assigned_static_partial

These are "= { }", "= { 0 }", and partial member initialization.

[09:44:47] [SKIPPED] test_small_hole_static_all
[09:44:47] [SKIPPED] test_small_hole_assigned_static_all
[09:44:47] [SKIPPED] test_small_hole_dynamic_all
[09:44:47] [SKIPPED] test_small_hole_assigned_dynamic_all

These are the full member initialization, which, yes, leaves the padding
uninitialized. :(

[09:44:47] [SKIPPED] test_small_hole_runtime_partial
[09:44:47] [SKIPPED] test_small_hole_runtime_all

These set members from runtime values instead of static values, and
padding is left uninitialized.

[09:44:47] [SKIPPED] test_small_hole_none

No init, obviously left uninitialized

[09:44:47] [PASSED] test_small_hole_assigned_copy

This is a full object copy (from a source with initialized padding),
so the result is also initialized.

Building with CONFIG_INIT_STACK_ALL_ZERO=y changes all this, of course.
In that case, everything passes:

./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y stackinit


-- 
Kees Cook