Message-ID: <99bc0165-814b-918f-1d62-4256836ab9a8@intel.com>
Date:   Wed, 1 Mar 2023 12:49:28 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 1/3] wifi: ipw2x00: convert ipw_fw_error->elem to flexible
 array[]
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     Kalle Valo <kvalo@kernel.org>, <linux-wireless@vger.kernel.org>,
        Stanislav Yakovlev <stas.yakovlev@gmail.com>
References: <20230228162827.3876606-1-jacob.e.keller@intel.com>
 <a8798dce4ae87aee64dfd56721b1668f8c969951.camel@sipsolutions.net>
 <d393ba90-ecdd-ffea-540b-d6db15571d5b@intel.com>
 <7f996d2efd23cefd17074edaeed0a6bbbd9f1a99.camel@sipsolutions.net>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <7f996d2efd23cefd17074edaeed0a6bbbd9f1a99.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R3pqMVcxYkJwU1JFa0JxcUM3MldPVzNwck93WTI1TW9DNU5Ua1djVmFaZGZS?=
 =?utf-8?B?OGdXWUdUL3V2TGtpcnpJN2tncTU3UElTQkFHR0o1UGNwd3hIdWp0UEpER2hY?=
 =?utf-8?B?L3BxMXpBWjB2NlRtQnFaQWZNVDV5dDRTbkpvZ0dNeWhNRS9sNWdXV0xxSnN1?=
 =?utf-8?B?UEtCNW5RLzBGMjFBdHBBMHJpeWhxWXRSK1lkYkZ4WGQwYzRPb0dKRFN5YTNV?=
 =?utf-8?B?N09sSTdxdXJUa0VZQVFDU1IwbStlOGhtRU92Wk03OGdIbFArU2FDeG9lTUZ0?=
 =?utf-8?B?emNHcHBsNmNQQUs4RTlTZDlYVjI5YS95UXZlN1hrNXRlZzZkSTNLTVcyamFB?=
 =?utf-8?B?d3dVeE1HL3BYZU9UQk9GR3ZXMTJkQmY5NTBzMElLVk5rK3daUlRaRWp0TzYz?=
 =?utf-8?B?STRZTDNDTnEyYU1wOEZXRksyeGFITk1zRkpQOHppK3JabzhCa3RMd2NobXFn?=
 =?utf-8?B?eU9DQ1pub1c4M2ZXUDFkbnRjSU9QbDRhNVI5a2xCc21tS0xhdmFKWkVsYmxW?=
 =?utf-8?B?NUVET0EvYnlVU3N6MjBrNVdlWDdPMzdocytTWmI1ODcyTDE1TTNrRHprZ3px?=
 =?utf-8?B?eVRPTUF1ZkUvTGRUQy9zYVRwVi91cXprSStTc0RYOGNyeFoycXg2THRWckJa?=
 =?utf-8?B?Lzdpd2JtVFZqK281SW9TakZQbVVRVURHakZJRHRHLzZVdDZKdnJjV2ozMnFs?=
 =?utf-8?B?WVVOWlVxb3dYejNrV3J6RWpuc0Q0c1kra2pOUm92a1E0b1BkRjFyY1ZPSkdR?=
 =?utf-8?B?c3d5U0ZaU0NTUWh4WXl1RHNpZk9UKzRHMEpDRUkrRGhZaUNiSDVWTlRnL2JP?=
 =?utf-8?B?UXo4T0l6Yk05N0FIZnNhMy9aakduMTlCdEhXNk81U1ByQjFEK1FBRm5ma016?=
 =?utf-8?B?QjNTNWdrZ2hGWGFDYTY4eTc4ZlA0Ky9acmhYU0V6ais0L0FqN0hTcTVVWXdl?=
 =?utf-8?B?aTZseHhCRlFLZFRBeTJtTldncVlZYTErcmpHSXBwMy84R3RGN0lZMHB2eEZq?=
 =?utf-8?B?UXkrMUdkRWtteS9RRmdmRzV2V3ByWWRDTHUvMFJVY1Jad1ZFZ0FPZmFWVWhS?=
 =?utf-8?B?Zi9FdC9OYXNCZDVzQ2ZuTDZNVWdRajJ5SmtSdm1zdmdkQ3kvejJqalhIaVg2?=
 =?utf-8?B?cEhFUlJxRmE5YTdKeS9PRURaVmhWemNoVjR0TGpBVEFVZnRGOXlFckVkNmR1?=
 =?utf-8?B?V01tclJZa29UQnBDZHRzQkVsOFUzREh2eXpXcjF1UEI3SjhRdjNpTW5VdW9T?=
 =?utf-8?B?VksyL2Z4TXgxWFhvd0JIN3U4Unk4Qm81NUNPQ1U0Zys3MEd0MmVZWHNob3Ay?=
 =?utf-8?B?OTgxVE90b1lBSEw0dGFNQUo0NmlycXZpUnMvYWVVdU9vRmJCZ0xmZFlzenRn?=
 =?utf-8?B?bHhPRW8raHFIazV1aFVLbGJTbFByWkIvOVFZTVQrUHVsa0hDRldtNmxvdG9s?=
 =?utf-8?B?QjBDdWdWM3FmdGFMdWRSODZubDgzcDRHOHpia1FDKzNlUDZ5OWZTbmVzUUc3?=
 =?utf-8?B?dVYrdG4vSHUwWXRCMDNucEwzRDV0UXBHVVhrak5FcnpIQjBTWDBzZUlKSnRa?=
 =?utf-8?B?dnVRZXRUeGo4aGFhSFVoa2NobTRzclNEc1NBK0tRZ1hkV0U4K1BsUVZkS25I?=
 =?utf-8?B?aXg4dHZMVWJRZmIzd2VQM1F4VkdTc0FDeGpYL1dpSGswam1XNHhCeWJubVE4?=
 =?utf-8?B?L21mOVdMejBGZGdqUy9JOEw2b1VXZW9HcEgvSlBtb256cFdMamtFRTJteDhv?=
 =?utf-8?B?a29zSGd4TTV2RXdPbkNmak82NzROemIzZmMwb016cWNTYXVCYXduKzhtTnVm?=
 =?utf-8?B?SW5DcHFIeWJZY2ZkVUI4b2x4MGk3cGhTd1h4VFVOcngzOWZsdjhyRk9mVkxv?=
 =?utf-8?B?T2JjcEYzWDRJa1BDcXA2T3NUZ09OVE9DRm5ESmVOcGgvSWVwZ2JHYTZaSkZt?=
 =?utf-8?B?bjkxbCs2UkdpS3pYT0k1S0MrVjlVOHM4b0RVWmlYcTFVdE9yNzhpVUxBVDND?=
 =?utf-8?B?SXpRL2RVbHFrVmdCZGlJUktLWVVMSzk0cUM0MzVKcm1NbmhUU2FFTEU2cWNx?=
 =?utf-8?B?OXEveUptZ2E4TktYZXJNYk5JZmQvR21vdXp5NzhiYVQ3RUpxVm9maVZGWm9C?=
 =?utf-8?B?SlJRb3U3R2gxVEN3K0lETlVtMEluanVvTFZIckFmdjZxSWZHaCs2RFJzVjdq?=
 =?utf-8?B?REE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 74822727-f792-40ff-ab94-08db1a967457
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Mar 2023 20:49:30.5656
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pzyyhtAWxLCr5QSeH2Q1XnSFOEUtuyKOOxhJLhCOQdlMuSH+A0uwtRbLcMBRYtF8uToBUjTJawx0RYDqOTvvSwkQ7VSo30zDuUKM+aFS7UI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6257
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org


On 2/28/2023 9:46 AM, Johannes Berg wrote:
> On Tue, 2023-02-28 at 09:44 -0800, Jacob Keller wrote:
>>
>> Previous to this change, error struct has two pointers to sections of
>> memory allocated at the end of the buffer.
>>
>> The code used to be:
>>
>> -	error = kmalloc(sizeof(*error) +
>> -			sizeof(*error->elem) * elem_len +
>> -			sizeof(*error->log) * log_len, GFP_ATOMIC);
>>
>> i.e. the elem_len is multiplying sizeof(*error->elem).
>>
>> The code is essentially trying to get two flexible arrays in the same
>> allocation, and its a bit messy to do that. I don't see how elem_len
>> could be anything other than "number of elems" given this code I removed.
> 
> Yeah, you're right. I was thinking of more modern HW/FW too much I
> guess, I see now even in the driver we have an array walk here (and it
> trusts the elem_len from firmware... ahrg!)
> 
>> I posted these mainly because I was trying to resolve all of the hits
>> that were found by the coccinelle patch I made, posted at [1]. I wanted
>> to get it to run clean so that we had no more struct_size hits.
>>
>> Dropping this would just make that patch have some hits until the driver
>> is removed, eventually...
>>
>> Not really a big deal to me, I just didn't want to post a coccinelle
>> patch without also trying to fix the handful of problems it reported,
>> since the total number of reports was small.
>>
> 
> Makes sense. I don't think we'll drop the driver at any point soon, but
> I also don't see it being changed much :)
> 
> johannes

I can drop this one out of the series if you don't have an intention of
taking it, or I can refactor to just use size_add and array_size without
converting it to flexible array, which would prevent that coccinelle
patch from complaining and at least ensure that we can't overflow size
and under-allocate.

Do you have a preference?

Thanks,
Jake