Am 10.11.20 um 18:11 schrieb Daniel Vetter:
On Tue, Nov 10, 2020 at 4:48 PM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Tue, Nov 10, 2020 at 03:24:32PM +1000, Dave Airlie wrote:
On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
Am 09.11.20 um 01:54 schrieb Dave Airlie:
@@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
if (bo->mem.mem_type != TTM_PL_SYSTEM) {
struct ttm_operation_ctx ctx = { false, false };
struct ttm_resource evict_mem;
+ struct ttm_place hop = {};
Please always use memset() if you want to zero initialize something in
the kernel, we had enough trouble with that.
What trouble is that? I've not heard of anything, and we use
={} quite extensively in drm land.
={} initializes only named fields, not padding.
Has that actually happened?
YES! Numerous times!
You wouldn't happen to have links/etc. to the discussion?
I'd like to check them out.
Uff, that was years ago. Just google for something like "mesa memset
hash", there was recently (~ the last year) another discussion because
somebody ran into exactly that problem once more.
Found this:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2F-%2Fissues%2F2071&data=04%7C01%7Cchristian.koenig%40amd.com%7C66be3d367f2b401b2e2808d8859ba4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637406250838085232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=X2z5zxsBvhXQvmd2oJzuN2YzDMGZnYUff6qxuJL%2BjLE%3D&reserved=0
which does suprise me a bit. Though I suspect ={} might
behave differently since the compiler might treat it
more like a memset().
It doesn't there's a bit of info out there on what happens, it really
only matters for structs we are passing to userspace, but it's
unlikely to matter here,
but I've changed this to memset in my local tree, because why not.
Structs passed to userspace should really have no implicit padding.
One of those how to botch your ioctl things...
The main problems with memset() are:
- it's ugly
- people often get the sizeof wrong
I guess the latter could be alleviated with a cpp macro that
does the sizeof correctly for you.
Yeah imo not using = {} and instead insisting on memset is really bad
w/a for not padding your api structs properly. memset is only one
place where this goes horribly wrong.
I'm not a fan of memset either, but there are more problems than just
the padding in the UAPI.
I've seen at least the following in the wild:
1. UAPI information leak.
2. Hash and fingerprinting functions returning unstable results even
when all meaningful members of a structure are initialized.
3. Valgrind/KASAN/Coverity randomly pointing this out as problem.
4. There is the discussion if ={} (not ISO C conform) or ={0] or even
={{0}} should be used.
My preference would be to just teach compilers that not initializing
padding in the kernel is a undesired behavior which should be avoided.
Regards
Christian.
I think for the gallium state tracker hasing issue memset is probably
ok ot use for these specifically, with a big comment explaining why.
And some code assurance that the memset is the same length you're
passing to the crc function and all that. But anywhere were it's more
(like anything related to api, which the gallium CSO hashing isnt) you
really want to have no implicit holes at all.
-Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
