Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH][next] scsi: aacraid: Replace one-element array with
 flexible-array member
To:     "Martin K. Petersen" <martin.petersen@oracle.com>
Cc:     "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Adaptec OEM Raid Solutions <aacraid@microsemi.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
References: <20210304203822.GA102218@embeddedor>
 <yq135wkm410.fsf@ca-mkp.ca.oracle.com>
 <668e3f30-1ffb-31e3-231b-705489993885@embeddedor.com>
 <yq1a6qqk68h.fsf@ca-mkp.ca.oracle.com>
From:   "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Message-ID: <d79bde59-16c5-e006-0e31-c33c17f0ce3d@embeddedor.com>
Date:   Thu, 25 Mar 2021 22:07:16 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <yq1a6qqk68h.fsf@ca-mkp.ca.oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Martin,


On 3/25/21 22:34, Martin K. Petersen wrote:
> 
> Gustavo,
> 
>> Precisely this sort of confusion is one of the things we want to avoid
>> by using flexible-array members instead of one-element arrays.
> 
> Ah, you're right!
> 
> Now that I look at it again I also don't think that was the issue that
> originally caused concern.
> 
> @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int
>  		}
>  	}
>  	sge[pos] = rio2->sge[nseg-1];
> -	memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212));
> +	memcpy(&rio2->sge[1], &sge[1],
> +	       flex_array_size(rio2, sge, nseg_new - 1));
>  
>  	kfree(sge);
>  	rio2->sgeCnt = cpu_to_le32(nseg_new);
> 
> I find it counter-intuitive to use the type of the destination array to
> size the amount of source data to copy. "Are source and destination same

The destination and source arrays are of the same type. :)

drivers/scsi/aacraid/aachba.c:
3999         struct sge_ieee1212 *sge;

> type? Does flex_array_size() do the right thing given the ->sge[1]
> destination offset?". It wasn't immediately obvious. To me, "copy this
> many scatterlist entries" in the original is much more readable.

Yeah; it does the right thing because flex_array_size() doesn't know about
offsets. It just calculates the amount of bytes to be copied based on the
type of the object passed as second argument and a "count" passed as third
argument. So, in this case, the "count" is "nseg_new - 1", which in some
way is already taking care of that sge[1] offset.

--
Gustavo