Message-ID: <d393ba90-ecdd-ffea-540b-d6db15571d5b@intel.com>
Date:   Tue, 28 Feb 2023 09:44:06 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
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>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <a8798dce4ae87aee64dfd56721b1668f8c969951.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?SWdpa2J1UitUQVNHY3Mzb1RmKzVxa0JkSlcxMEhNQ0lOemRHbERFaDBrZTc4?=
 =?utf-8?B?OFVEMm9iWjFJRHFjbWs5aVFSY1lvbzFFZ0tIUExxWWtNZzZHTGdnMkNnUER2?=
 =?utf-8?B?QndXREQyNWVseEE1VjdGQkRyek1BczNqS3YxdmlORGVnRy9BWTVrR3VybUd5?=
 =?utf-8?B?M2FVbFRkcG5BWGZLLzRmVzlpdnR2SFlSbW40TUsxRTlJOHN3M2RLQXZTRU5R?=
 =?utf-8?B?ZU02a2VUWlJycVZLSW04Yk5qMXJzT0YrTFhLYzY0Z2V6cEJjaGpISGh3ZldT?=
 =?utf-8?B?ejdhNlVSdUt4NnMvdWNQdmxGSWJHaTA1eWdaTEFITXk2RHV0emh5cFpsc3NT?=
 =?utf-8?B?RmtCMEV0TnF3RE9GcGpJSkVZZEcyYlJIUThHRXVzTTVTQ3FLZXBDVnd6QW0r?=
 =?utf-8?B?ZC9nU0pkdUY4eGxvNjVkb1A1MkJXUGVMRzBsZ1cyb25hVFliTG5EM1FIVHR5?=
 =?utf-8?B?dnoxM01jYmFCWXZ1ZklzT3Y3L0prbWNUZHgySG1JZGJIR2pFYzViRTlUaFB6?=
 =?utf-8?B?eFZFTVdnZmxWVldFNU9BY1lEVHZuT2RHbC9nV0IreCtUWCt3emt1SUxVVUhi?=
 =?utf-8?B?a0xkQ2VpUFRONXJRNktJME56c0wvcU9SUXdXR3BhVUlMMmFSNDFGOXpXa3NT?=
 =?utf-8?B?S3p2YnZLcFVndGxCMndnTjk2Nk0vVGR2ckVuVS9EZUhxaVUzeE9TWjg5bnJr?=
 =?utf-8?B?UmRtT3hOeE9SdDFLWTREUkVNbk9PdlVDN0hDTkg4N05ZeE5hRHpFQUM4Y3pq?=
 =?utf-8?B?QzNGdWd6NFNvWU1rVjhTemhQRitqbVgxMjlXMGxaamIxSUl0aWljU214dHBy?=
 =?utf-8?B?b2VrU2VUeERjSGJ3bzZpaW5uK0hRdmppQ1RSMUNTcU9IWUt3UzBYRkdqU2xl?=
 =?utf-8?B?bHpYVkZzeFQvZ3drNGR3MTN5aHBNRjRHVFRkdCtWVVBhenJOeWpUbEZ2cTZJ?=
 =?utf-8?B?WklHZEZrWE1Xd05BY0pISUZHQlRpL1cya3dTenFFYlZ3bTcvRHEySnBUSGh1?=
 =?utf-8?B?STNWS3R2RHZuYk9ZZW5NeFg1dW9FcStwaFJ1RUtsaE1nWmdpUUJ5aGoyaCt6?=
 =?utf-8?B?b3lQOVJTWnBGYmFEQ1ZhMERRVFFncExvbEE4TUV5a24yZDVIZFM3Z000UW8x?=
 =?utf-8?B?ZmdDbjcwWllnOTlYSWVrcDB4cXRRZUx5YUhBNGFwVzc3bnpHNjVWSXFwSzR1?=
 =?utf-8?B?SFVUQ0xqdzEzNmg1SytKeWxHRnl2bmx4S0duQXU2aEw2RkppMXMxbzh5dnJQ?=
 =?utf-8?B?b1lweG1YQ05OL3BPUC96OEpBL3E1LzlwTDNnUFhRZWtqOG9JMDdOZ2dqNVhI?=
 =?utf-8?B?bDJlbEU3NmF2T2NVcnphL1hRdkZ1TlQvb3Ywa2VEZWUzbWZKRWtramhsQnNT?=
 =?utf-8?B?SXNmUktSR2Q3bEh2TG8ranZZK3A3OUlTM3d0U1U1VU1oR29kOElpdndqY1kx?=
 =?utf-8?B?OEZxNkJ0ZGdMMFlOZU5hcmU0dkVZSXVDYjNsTGhWdVFWclg3c3NLMFRoczBS?=
 =?utf-8?B?djhqb2gvU3A3WEFXV1N6OGVGVE5DNGZKMVBaRmJiMjJZUXg5RjZIRnJmMGE2?=
 =?utf-8?B?aEQwSFpWUUNLUHJzb1hRNnVrL2o0cThrTnkvWFhkb0JNQmF2SmRwcHZ6dk1D?=
 =?utf-8?B?VzR6c1VsYkNGK0Rka3haaE9zWDZVOHUwTFBiMXVpb2ZQQmF3M1lOZmVEaEJS?=
 =?utf-8?B?UmR5eFhPM2Q4S0ZVNHhkbVBhQlBwMldtY2xoTERBdVk0RzE3b1JvR2wyMkcw?=
 =?utf-8?B?amlEM282L0owNkRpbjdhdmorUS9ySHg3YkhESDh4Vk9vaXR6VDZkQmgrKytn?=
 =?utf-8?B?SGZhbUFrNDlnQy9sRHY2WEhwUGRLUnlhRTduSUtFb2NNS1JKMGE5REdFUUhk?=
 =?utf-8?B?WFRZMy9TbTJCN1dFV0VwQms4aWZLUitjMkFZL1g3Z3VIbFNZYkJUeWFDS2hW?=
 =?utf-8?B?NG9SZWxzNGlqS1pTVm8wUXhhWG5jUWdXNm85S1hFeklLMEFjdXRKSklPaFFk?=
 =?utf-8?B?QmVVKzlNWlNiVHVYZ2pWd1Vpa21uaWVNa0FlQWcyeWdDaVoxZ2RMY3JlaXg0?=
 =?utf-8?B?Ly9DbkRhRnF2Y1ZibGU3YTQvalI0UjNRa3cwb0pRSXgwOW5DaTBtcG1EZGVD?=
 =?utf-8?B?NEp2dG5DUlBJczl4bEJKajdNdXBwa3JrNmxlWEF0d1BJM2oybVFUaXF6UHE3?=
 =?utf-8?B?ZEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bcc20f02-c0c5-4b7a-1470-08db19b364b7
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Feb 2023 17:44:08.4648
 (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: btG7s/OerlsgNFW3si0zJzgauAyjA7N6VmuPgkc9/oaX7D8tKGa5fGRgJq15kswcGw2uxdzZJhVeoAuVtgKjkl3rTrrWaDtFdsawhOEkzLY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB8076
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:16 AM, Johannes Berg wrote:
> On Tue, 2023-02-28 at 08:28 -0800, Jacob Keller wrote:
>>
>> @@ -1247,7 +1247,6 @@ static struct ipw_fw_error *ipw_alloc_error_log(struct ipw_priv *priv)
>>  	error->config = priv->config;
>>  	error->elem_len = elem_len;
>>  	error->log_len = log_len;
>> -	error->elem = (struct ipw_error_elem *)error->payload;
>>  	error->log = (struct ipw_event *)(error->elem + elem_len);
> 
> I really don't know this driver, it's ancient, but that last line looks
> wrong to me already, elem_len doesn't seem like # of elems?
> 
> But I guess this patch changes nothing here, so hey. Don't think there's
> much value in the change either, this driver isn't going to get touched
> any more, just removed eventually ;)
> 
> johannes
> 

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.

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.

Thanks,
Jake

[1]:
https://lore.kernel.org/all/20230227202428.3657443-1-jacob.e.keller@intel.com/