| Message ID | 20211003104258.18550-1-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drm/i915: Prefer struct_size over open coded arithmetic | expand |
Hi, On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > As noted in the "Deprecated Interfaces, Language Features, Attributes, > and Conventions" documentation [1], size calculations (especially > multiplication) should not be performed in memory allocator (or similar) > function arguments due to the risk of them overflowing. This could lead > to values wrapping around and a smaller allocation being made than the > caller was expecting. Using those allocations could lead to linear > overflows of heap memory and other misbehaviors. > > In this case these are not actually dynamic sizes: all the operands > involved in the calculation are constant values. However it is better to > refactor them anyway, just to keep the open-coded math idiom out of > code. > > So, add at the end of the struct i915_syncmap a union with two flexible > array members (these arrays share the same memory layout). This is > possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > struct_size() helper to do the arithmetic instead of the argument > "size + count * size" in the kmalloc and kzalloc() functions. > > Also, take the opportunity to refactor the __sync_seqno and __sync_child > making them more readable. > > This code was detected with the help of Coccinelle and audited and fixed > manually. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > Signed-off-by: Len Baker <len.baker@gmx.com> > --- > drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) I received a mail telling that this patch doesn't build: == Series Details == Series: drm/i915: Prefer struct_size over open coded arithmetic URL : https://patchwork.freedesktop.org/series/95408/ State : failure But it builds without error against linux-next (tag next-20211001). Against which tree and branch do I need to build? Regards, Len
On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > Hi, > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: >> As noted in the "Deprecated Interfaces, Language Features, Attributes, >> and Conventions" documentation [1], size calculations (especially >> multiplication) should not be performed in memory allocator (or similar) >> function arguments due to the risk of them overflowing. This could lead >> to values wrapping around and a smaller allocation being made than the >> caller was expecting. Using those allocations could lead to linear >> overflows of heap memory and other misbehaviors. >> >> In this case these are not actually dynamic sizes: all the operands >> involved in the calculation are constant values. However it is better to >> refactor them anyway, just to keep the open-coded math idiom out of >> code. >> >> So, add at the end of the struct i915_syncmap a union with two flexible >> array members (these arrays share the same memory layout). This is >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the >> struct_size() helper to do the arithmetic instead of the argument >> "size + count * size" in the kmalloc and kzalloc() functions. >> >> Also, take the opportunity to refactor the __sync_seqno and __sync_child >> making them more readable. >> >> This code was detected with the help of Coccinelle and audited and fixed >> manually. >> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments >> >> Signed-off-by: Len Baker <len.baker@gmx.com> >> --- >> drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) > > I received a mail telling that this patch doesn't build: > > == Series Details == > > Series: drm/i915: Prefer struct_size over open coded arithmetic > URL : https://patchwork.freedesktop.org/series/95408/ > State : failure > > But it builds without error against linux-next (tag next-20211001). Against > which tree and branch do I need to build? drm-tip [1]. It's a sort of linux-next for graphics. I think there are still some branches that don't feed to linux-next. BR, Jani. [1] https://cgit.freedesktop.org/drm/drm-tip > > Regards, > Len
On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: > On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > > Hi, > > > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > >> and Conventions" documentation [1], size calculations (especially > >> multiplication) should not be performed in memory allocator (or similar) > >> function arguments due to the risk of them overflowing. This could lead > >> to values wrapping around and a smaller allocation being made than the > >> caller was expecting. Using those allocations could lead to linear > >> overflows of heap memory and other misbehaviors. > >> > >> In this case these are not actually dynamic sizes: all the operands > >> involved in the calculation are constant values. However it is better to > >> refactor them anyway, just to keep the open-coded math idiom out of > >> code. > >> > >> So, add at the end of the struct i915_syncmap a union with two flexible > >> array members (these arrays share the same memory layout). This is > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > >> struct_size() helper to do the arithmetic instead of the argument > >> "size + count * size" in the kmalloc and kzalloc() functions. > >> > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child > >> making them more readable. > >> > >> This code was detected with the help of Coccinelle and audited and fixed > >> manually. > >> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > >> > >> Signed-off-by: Len Baker <len.baker@gmx.com> > >> --- > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- > >> 1 file changed, 8 insertions(+), 4 deletions(-) > > > > I received a mail telling that this patch doesn't build: > > > > == Series Details == > > > > Series: drm/i915: Prefer struct_size over open coded arithmetic > > URL : https://patchwork.freedesktop.org/series/95408/ > > State : failure > > > > But it builds without error against linux-next (tag next-20211001). Against > > which tree and branch do I need to build? > > drm-tip [1]. It's a sort of linux-next for graphics. I think there are > still some branches that don't feed to linux-next. Yeah we need to get gt-next in linux-next asap. Joonas promised to send out his patch to make that happen in dim. -Daniel > > BR, > Jani. > > > [1] https://cgit.freedesktop.org/drm/drm-tip > > > > > > Regards, > > Len > > -- > Jani Nikula, Intel Open Source Graphics Center
Hi Daniel and Jani, On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote: > On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: > > On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > > > Hi, > > > > > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > > >> and Conventions" documentation [1], size calculations (especially > > >> multiplication) should not be performed in memory allocator (or similar) > > >> function arguments due to the risk of them overflowing. This could lead > > >> to values wrapping around and a smaller allocation being made than the > > >> caller was expecting. Using those allocations could lead to linear > > >> overflows of heap memory and other misbehaviors. > > >> > > >> In this case these are not actually dynamic sizes: all the operands > > >> involved in the calculation are constant values. However it is better to > > >> refactor them anyway, just to keep the open-coded math idiom out of > > >> code. > > >> > > >> So, add at the end of the struct i915_syncmap a union with two flexible > > >> array members (these arrays share the same memory layout). This is > > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > > >> struct_size() helper to do the arithmetic instead of the argument > > >> "size + count * size" in the kmalloc and kzalloc() functions. > > >> > > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child > > >> making them more readable. > > >> > > >> This code was detected with the help of Coccinelle and audited and fixed > > >> manually. > > >> > > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > >> > > >> Signed-off-by: Len Baker <len.baker@gmx.com> > > >> --- > > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- > > >> 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > I received a mail telling that this patch doesn't build: > > > > > > == Series Details == > > > > > > Series: drm/i915: Prefer struct_size over open coded arithmetic > > > URL : https://patchwork.freedesktop.org/series/95408/ > > > State : failure > > > > > > But it builds without error against linux-next (tag next-20211001). Against > > > which tree and branch do I need to build? > > > > drm-tip [1]. It's a sort of linux-next for graphics. I think there are > > still some branches that don't feed to linux-next. > > Yeah we need to get gt-next in linux-next asap. Joonas promised to send > out his patch to make that happen in dim. > -Daniel Is there a possibility that you give an "Acked-by" tag? And then this patch goes to the mainline through the Kees' tree or Gustavo's tree? Or is it better to wait for drm-tip to update? Regards, Len > > > > > BR, > > Jani. > > > > > > [1] https://cgit.freedesktop.org/drm/drm-tip > > > > > > > > > > Regards, > > > Len > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Sat, 16 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > Hi Daniel and Jani, > > On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote: >> On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: >> > On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote: >> > > Hi, >> > > >> > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: >> > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, >> > >> and Conventions" documentation [1], size calculations (especially >> > >> multiplication) should not be performed in memory allocator (or similar) >> > >> function arguments due to the risk of them overflowing. This could lead >> > >> to values wrapping around and a smaller allocation being made than the >> > >> caller was expecting. Using those allocations could lead to linear >> > >> overflows of heap memory and other misbehaviors. >> > >> >> > >> In this case these are not actually dynamic sizes: all the operands >> > >> involved in the calculation are constant values. However it is better to >> > >> refactor them anyway, just to keep the open-coded math idiom out of >> > >> code. >> > >> >> > >> So, add at the end of the struct i915_syncmap a union with two flexible >> > >> array members (these arrays share the same memory layout). This is >> > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the >> > >> struct_size() helper to do the arithmetic instead of the argument >> > >> "size + count * size" in the kmalloc and kzalloc() functions. >> > >> >> > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child >> > >> making them more readable. >> > >> >> > >> This code was detected with the help of Coccinelle and audited and fixed >> > >> manually. >> > >> >> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments >> > >> >> > >> Signed-off-by: Len Baker <len.baker@gmx.com> >> > >> --- >> > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- >> > >> 1 file changed, 8 insertions(+), 4 deletions(-) >> > > >> > > I received a mail telling that this patch doesn't build: >> > > >> > > == Series Details == >> > > >> > > Series: drm/i915: Prefer struct_size over open coded arithmetic >> > > URL : https://patchwork.freedesktop.org/series/95408/ >> > > State : failure >> > > >> > > But it builds without error against linux-next (tag next-20211001). Against >> > > which tree and branch do I need to build? >> > >> > drm-tip [1]. It's a sort of linux-next for graphics. I think there are >> > still some branches that don't feed to linux-next. >> >> Yeah we need to get gt-next in linux-next asap. Joonas promised to send >> out his patch to make that happen in dim. >> -Daniel > > Is there a possibility that you give an "Acked-by" tag? And then this patch > goes to the mainline through the Kees' tree or Gustavo's tree? If this does not apply to drm-intel-gt-next (or drm-tip), applying it to some other branch will just cause unnecessary conflicts later on. It's unnecessary extra work. It's not an urgent fix or anything, there is no reason to do that. So that's a NAK. > Or is it better to wait for drm-tip to update? drm-tip is up to date, it's just that one of the branches that feed to it is (was?) not feeding to linux-next. If you're contributing to drm, please consider basing your patches on top of drm-tip. BR, Jani. > > Regards, > Len > >> >> > >> > BR, >> > Jani. >> > >> > >> > [1] https://cgit.freedesktop.org/drm/drm-tip >> > >> > >> > > >> > > Regards, >> > > Len >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch
Hi Jani, On Mon, Oct 18, 2021 at 01:00:01PM +0300, Jani Nikula wrote: > On Sat, 16 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > > Hi Daniel and Jani, > > > > On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote: > >> On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: > >> > On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > >> > > Hi, > >> > > > >> > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > >> > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > >> > >> and Conventions" documentation [1], size calculations (especially > >> > >> multiplication) should not be performed in memory allocator (or similar) > >> > >> function arguments due to the risk of them overflowing. This could lead > >> > >> to values wrapping around and a smaller allocation being made than the > >> > >> caller was expecting. Using those allocations could lead to linear > >> > >> overflows of heap memory and other misbehaviors. > >> > >> > >> > >> In this case these are not actually dynamic sizes: all the operands > >> > >> involved in the calculation are constant values. However it is better to > >> > >> refactor them anyway, just to keep the open-coded math idiom out of > >> > >> code. > >> > >> > >> > >> So, add at the end of the struct i915_syncmap a union with two flexible > >> > >> array members (these arrays share the same memory layout). This is > >> > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > >> > >> struct_size() helper to do the arithmetic instead of the argument > >> > >> "size + count * size" in the kmalloc and kzalloc() functions. > >> > >> > >> > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child > >> > >> making them more readable. > >> > >> > >> > >> This code was detected with the help of Coccinelle and audited and fixed > >> > >> manually. > >> > >> > >> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > >> > >> > >> > >> Signed-off-by: Len Baker <len.baker@gmx.com> > >> > >> --- > >> > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- > >> > >> 1 file changed, 8 insertions(+), 4 deletions(-) > >> > > > >> > > I received a mail telling that this patch doesn't build: > >> > > > >> > > == Series Details == > >> > > > >> > > Series: drm/i915: Prefer struct_size over open coded arithmetic > >> > > URL : https://patchwork.freedesktop.org/series/95408/ > >> > > State : failure > >> > > > >> > > But it builds without error against linux-next (tag next-20211001). Against > >> > > which tree and branch do I need to build? > >> > > >> > drm-tip [1]. It's a sort of linux-next for graphics. I think there are > >> > still some branches that don't feed to linux-next. > >> > >> Yeah we need to get gt-next in linux-next asap. Joonas promised to send > >> out his patch to make that happen in dim. > >> -Daniel > > > > Is there a possibility that you give an "Acked-by" tag? And then this patch > > goes to the mainline through the Kees' tree or Gustavo's tree? > > If this does not apply to drm-intel-gt-next (or drm-tip), applying it to > some other branch will just cause unnecessary conflicts later on. It's > unnecessary extra work. It's not an urgent fix or anything, there is no > reason to do that. So that's a NAK. Ok. Understood. > > Or is it better to wait for drm-tip to update? > > drm-tip is up to date, it's just that one of the branches that feed to > it is (was?) not feeding to linux-next. Sorry, but I'm missing something here. In linux-next this is the commit history of include/linux/stddef.h file: 3080ea5553cc stddef: Introduce DECLARE_FLEX_ARRAY() helper 50d7bd38c3aa stddef: Introduce struct_group() helper macro e7f18c22e6be stddef: Fix kerndoc for sizeof_field() and offsetofend() 4229a470175b stddef.h: Introduce sizeof_field() ... But in drm-tip this is the commit history: 4229a470175b stddef.h: Introduce sizeof_field() ... For this patch the DECLARE_FLEX_ARRAY() helper is needed. But the build fails due to the last tree commits for stddef.h file are not present. So, if I understand correctly, drm-tip is not up to date with linux-next. Regards, Len > > If you're contributing to drm, please consider basing your patches on > top of drm-tip. > > > BR, > Jani. > > > > > > Regards, > > Len > > > >> > >> > > >> > BR, > >> > Jani. > >> > > >> > > >> > [1] https://cgit.freedesktop.org/drm/drm-tip > >> > > >> > > >> > > > >> > > Regards, > >> > > Len > >> > > >> > -- > >> > Jani Nikula, Intel Open Source Graphics Center > >> > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> http://blog.ffwll.ch > > -- > Jani Nikula, Intel Open Source Graphics Center
On Sat, 23 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > Sorry, but I'm missing something here. In linux-next this is the commit > history of include/linux/stddef.h file: > > 3080ea5553cc stddef: Introduce DECLARE_FLEX_ARRAY() helper > 50d7bd38c3aa stddef: Introduce struct_group() helper macro > e7f18c22e6be stddef: Fix kerndoc for sizeof_field() and offsetofend() > 4229a470175b stddef.h: Introduce sizeof_field() > ... > > But in drm-tip this is the commit history: > > 4229a470175b stddef.h: Introduce sizeof_field() > ... > > For this patch the DECLARE_FLEX_ARRAY() helper is needed. But the build > fails due to the last tree commits for stddef.h file are not present. > So, if I understand correctly, drm-tip is not up to date with linux-next. linux-next is an ephemeral integration branch for most arch, subsystem and driver -next branches. drm-tip is an ephemeral integration branch for drm subsystem and driver -next branches. They contain different sets of branches. They are constantly rebuilt. They are not the end result or end goal. If a problem (or a solution, for that matter) only exists in the merge of some of those branches, you can't actually fix it until such a merge exists somewhere more permanent than an ephemeral integration branch. BR, Jani.
Hi, On Wed, Oct 27, 2021 at 06:06:28PM +0300, Jani Nikula wrote: > On Sat, 23 Oct 2021, Len Baker <len.baker@gmx.com> wrote: > > Sorry, but I'm missing something here. In linux-next this is the commit > > history of include/linux/stddef.h file: > > > > 3080ea5553cc stddef: Introduce DECLARE_FLEX_ARRAY() helper > > 50d7bd38c3aa stddef: Introduce struct_group() helper macro > > e7f18c22e6be stddef: Fix kerndoc for sizeof_field() and offsetofend() > > 4229a470175b stddef.h: Introduce sizeof_field() > > ... > > > > But in drm-tip this is the commit history: > > > > 4229a470175b stddef.h: Introduce sizeof_field() > > ... > > > > For this patch the DECLARE_FLEX_ARRAY() helper is needed. But the build > > fails due to the last tree commits for stddef.h file are not present. > > So, if I understand correctly, drm-tip is not up to date with linux-next. > > linux-next is an ephemeral integration branch for most arch, subsystem > and driver -next branches. > > drm-tip is an ephemeral integration branch for drm subsystem and driver > -next branches. > > They contain different sets of branches. They are constantly > rebuilt. They are not the end result or end goal. > > If a problem (or a solution, for that matter) only exists in the merge > of some of those branches, you can't actually fix it until such a merge > exists somewhere more permanent than an ephemeral integration branch. Ok, understood. Thanks for the clarification. Regards, Len
diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c index 60404dbb2e9f..a8d35491d05a 100644 --- a/drivers/gpu/drm/i915/i915_syncmap.c +++ b/drivers/gpu/drm/i915/i915_syncmap.c @@ -82,6 +82,10 @@ struct i915_syncmap { * struct i915_syncmap *child[KSYNCMAP]; * }; */ + union { + DECLARE_FLEX_ARRAY(u32, seqno); + DECLARE_FLEX_ARRAY(struct i915_syncmap *, child); + }; }; /** @@ -99,13 +103,13 @@ void i915_syncmap_init(struct i915_syncmap **root) static inline u32 *__sync_seqno(struct i915_syncmap *p) { GEM_BUG_ON(p->height); - return (u32 *)(p + 1); + return p->seqno; } static inline struct i915_syncmap **__sync_child(struct i915_syncmap *p) { GEM_BUG_ON(!p->height); - return (struct i915_syncmap **)(p + 1); + return p->child; } static inline unsigned int @@ -200,7 +204,7 @@ __sync_alloc_leaf(struct i915_syncmap *parent, u64 id) { struct i915_syncmap *p; - p = kmalloc(sizeof(*p) + KSYNCMAP * sizeof(u32), GFP_KERNEL); + p = kmalloc(struct_size(p, seqno, KSYNCMAP), GFP_KERNEL); if (unlikely(!p)) return NULL; @@ -282,7 +286,7 @@ static noinline int __sync_set(struct i915_syncmap **root, u64 id, u32 seqno) unsigned int above; /* Insert a join above the current layer */ - next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next), + next = kzalloc(struct_size(next, child, KSYNCMAP), GFP_KERNEL); if (unlikely(!next)) return -ENOMEM;
As noted in the "Deprecated Interfaces, Language Features, Attributes, and Conventions" documentation [1], size calculations (especially multiplication) should not be performed in memory allocator (or similar) function arguments due to the risk of them overflowing. This could lead to values wrapping around and a smaller allocation being made than the caller was expecting. Using those allocations could lead to linear overflows of heap memory and other misbehaviors. In this case these are not actually dynamic sizes: all the operands involved in the calculation are constant values. However it is better to refactor them anyway, just to keep the open-coded math idiom out of code. So, add at the end of the struct i915_syncmap a union with two flexible array members (these arrays share the same memory layout). This is possible using the new DECLARE_FLEX_ARRAY macro. And then, use the struct_size() helper to do the arithmetic instead of the argument "size + count * size" in the kmalloc and kzalloc() functions. Also, take the opportunity to refactor the __sync_seqno and __sync_child making them more readable. This code was detected with the help of Coccinelle and audited and fixed manually. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) -- 2.25.1