linux-kernel - Re: [PATCH] drm/i915: change node clearing from memset to initialization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <715d6dbd696a9dad2690772d175d2ab497e0c316.camel@perches.com>
Date:   Sat, 16 Apr 2022 14:04:24 -0700
From:   Joe Perches <joe@...ches.com>
To:     Tom Rix <trix@...hat.com>, jani.nikula@...ux.intel.com,
        joonas.lahtinen@...ux.intel.com, rodrigo.vivi@...el.com,
        tvrtko.ursulin@...ux.intel.com, airlied@...ux.ie, daniel@...ll.ch
Cc:     intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/i915: change node clearing from memset to
 initialization

On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
> On 4/16/22 11:33 AM, Joe Perches wrote:
> > On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> > > In insert_mappable_node(), the parameter node is
> > > cleared late in node's use with memset.
> > > insert_mappable_node() is a singleton, called only
> > > from i915_gem_gtt_prepare() which itself is only
> > > called by i915_gem_gtt_pread() and
> > > i915_gem_gtt_pwrite_fast() where the definition of
> > > node originates.
> > > 
> > > Instead of using memset, initialize node to 0 at it's
> > > definitions.
> > trivia: /it's/its/
> > 
> > Only reason _not_ to do this is memset is guaranteed to
> > zero any padding that might go to userspace.
> > 
> > But it doesn't seem there is any padding anyway nor is
> > the struct available to userspace.
> > 
> > So this seems fine though it might increase overall code
> > size a tiny bit.
> > 
> > I do have a caveat: see below:
> > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > []
> > > @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct drm_i915_gem_object *obj,
> > >   		goto err_ww;
> > >   	} else if (!IS_ERR(vma)) {
> > >   		node->start = i915_ggtt_offset(vma);
> > > -		node->flags = 0;
> > Why is this unneeded?
> 
> node = {} initializes all of node's elements to 0, including this one.

true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?