Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7cf14d1b-1a3a-fea7-0d2c-409ab828569b@oracle.com>
Date:   Tue, 4 Jul 2023 17:11:42 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] module: always complete idempotent loads
Content-Language: en-US
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        George Kennedy <george.kennedy@oracle.com>,
        linux-kernel@vger.kernel.org, linux-modules@vger.kernel.org,
        Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
        syzbot+9c2bdc9d24e4a7abe741@syzkaller.appspotmail.com,
        Johan Hovold <johan@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Rudi Heitbaum <rudi@heitbaum.com>,
        David Hildenbrand <david@redhat.com>
References: <20230704092309.22669-1-vegard.nossum@oracle.com>
 <20230704100852.23452-1-vegard.nossum@oracle.com>
 <CAHk-=wgy2q9viwq-bAqaq8X_WQHNqDsu1AzfeYXAcDQRZmCoAw@mail.gmail.com>
From:   Vegard Nossum <vegard.nossum@oracle.com>
In-Reply-To: <CAHk-=wgy2q9viwq-bAqaq8X_WQHNqDsu1AzfeYXAcDQRZmCoAw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NERiUkNZdytyNC80dmZVZi8wVmV4YzIvN3ZSeFlsdXk3NmZtcEpPbHpWdVox?=
 =?utf-8?B?ZmYrbnJsWXRWM2dCS3JnSVdBblhDY3IzTGYyU01jK010Qlg0VXhRdm5zSEc4?=
 =?utf-8?B?ZWU5WGZWVUJvcVlXMlV6Smg2SC9VVHVRSTlTQjgvM3hhOUV5NktyQVhCQzgx?=
 =?utf-8?B?dGhtR1pCSE5RbW14SzNwT3B4elZ4T0ZaVElxakxuNFRuYWJuaENYQ3VaUFZw?=
 =?utf-8?B?aWZndmJCSFV5MWl4dXFXaVc1UExJbDlCcHhxMitiWndnY2htdEZVaEJaQ2JC?=
 =?utf-8?B?Q3ZXbHk2TUhad2VmdEZpTVRGckxVU09ibDVsK3Z6RVQ1bm9rOEw2SEN1cUlM?=
 =?utf-8?B?dmgxNmhtMTY1TkhNWXhOc1NKSkRJelRwYzRTZVcwcmhidGJiSGoxb0hYcGg5?=
 =?utf-8?B?QlcyOXVkYjZYVi8vNUhJbTdBT0tOSERLd3Y3RW1QdmsvNkJIYzZJY2JXREh0?=
 =?utf-8?B?cXZHZG5qNlpBaGdKUUllbHUxbHRVbTQ1aWtSU2tVR0VwQTJXZlQzVXJxVkV2?=
 =?utf-8?B?b1hkcWwzbXZxTWpVQ0FBeHBNL0tJYlV2a3I4NUs5TmhFck5maHVlNXBDUWVz?=
 =?utf-8?B?cG54V2hKcEdOeUtMTGZEZkFWb1dJQVpYRHdKZi9zWFBPV1E1ZHdRTm8vZkpz?=
 =?utf-8?B?bnEzZjNxUFFEVmpVQm9YM0x6TU0yNFdiY0pRY1VCb214U29acW1zeXh6WGoz?=
 =?utf-8?B?NHFzWjlvN1NCRmNSTFpqaDZoVXN2bzdFYkRvaFRKSmFvTW9aZCtYSzA1aVVk?=
 =?utf-8?B?cmUzZm9JZ2FSYXdEY01wdzYxQ0RZU3Jid1RMSDEzcGJ1bFhYanJXUFVLVjJP?=
 =?utf-8?B?ZHI5V1J1RllWL2ptNkN5NGR6S3NYdXh4QWhSUERXM1pPTjI4VlE0ZTkvdExP?=
 =?utf-8?B?WXl6czgwZk9PWEpLZ0dvMy9ZNmR5dUZZdjBUVThqM2wyZHFIaHJZdzdNWmtI?=
 =?utf-8?B?WGtZUUZwQURFSFFKWEJoOHVoajlIT1kvQjdRM09VU09JcnA4VFdLMGRoS1dB?=
 =?utf-8?B?NmNUcXp4bTM2U2VMcXJZWnEwSnY1ckoxb3NoM1ZFUmZlTXlnamxXSU1tZmZn?=
 =?utf-8?B?UzZlK1hCVENuME5jSU1RaHFHazAxaFd5Y1diK2VIQ3dXYUx4YXV5dGxwK25P?=
 =?utf-8?B?dFdHOTl4WldFTDQ3UmRlcTdkdGtZam4vOUo0V0ZkcHoyWHBjdlFMblBiTFNQ?=
 =?utf-8?B?MUZWeGtCbGlQaFZaL25Bbm9ncDNWUERGc0EwRk55am5sakFETm90OGtSMGFH?=
 =?utf-8?B?YUxyYnZtZFVRb0pCV3lhVW81YWhHeUtUd09wS003SVVxQm5SdVRZNGo4ejFa?=
 =?utf-8?B?WlJ2ZGZqN2lDeG04aUU3c2xUb09zUXBpRHhSQjFnZ241dE9qOHltQWRRQUpL?=
 =?utf-8?B?ZjBuaEdpL25vdDNYSlN0Qi9xK0dvUzlqM2o3N1dEdVo3QU1lclFLSDNHYVFh?=
 =?utf-8?B?S29SeDJrNXpWYmFuU3cwRDBNR3I0dmRJSkVYYzd3a3JINktCOVBodE5sdUhW?=
 =?utf-8?B?RTYzRlFEOVV4emNQb0Z4cCtvdU9yOWcvdzhPWUxhbnRiYmx4TG9Scy9DTFRN?=
 =?utf-8?B?VGh4cnRNaGJjUUxia3phU3lRa1lvQWZlcXJsMzg0NmxuUFZsZHpEMmZBZmZh?=
 =?utf-8?B?UVhmZE9WK0NyMG0yTWh0dXpsaHRoSVNvK2tFNVlmTU4rb2NMYU9yR3Fjbmcr?=
 =?utf-8?B?bm1yZUVUcG92WFpMM1lINUtIZkw5c2krSGEzbjNMRDd5bmlGMnFoS1h3bFBt?=
 =?utf-8?B?cE5XaWtGZjF6djJ1dXZ1UzJMVG9zZS92OGljRkFnSDRiRlc4RXZTL0VMRW9L?=
 =?utf-8?B?MTk5M3lQcGE3WFFQeGxTTDJNeEtFZm5NVVZzd210L2tHTDVTSDRtWmNqMU5W?=
 =?utf-8?B?NVh5NXgwdlpQcDIwWnhkcUs5OVppY2JXcmRIUGg2N3M2SkxUTXdodHVRZjd2?=
 =?utf-8?B?akJWVndmSHF2NjFzRDRsUDRnVWVQOTYrTjMzNmhITGJJVlBVRlVHYlpIUVQy?=
 =?utf-8?B?VmRrRUZjc1J3OW5zcFZqN1d6R1JDYTh1bVU1MEp5VUtaVkkxdXhaeWxhb0lK?=
 =?utf-8?B?T1lTRHNLRDk5c3NPcXQ1VjlKc0QyVHJ4RDV6a2pOVzRNdEp4b3ZKVjcyTCtG?=
 =?utf-8?Q?CQKFPn4KBMoJoqA5AfWw9xPyM?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?OXZmclh2Q0d6T2xjekdoWTkyajFtTWFhRzdETkxkQ1VSNDZxeFI1L1BZMWRQ?=
 =?utf-8?B?K01mY01HVnBpSnI1OU5takVVZlN3NlE0VTdTc1JBOHh5NjN0SkFEOThPdmNt?=
 =?utf-8?B?QlREMktPTDNOTHgvd2dLTVliNmpZV2hRbTdIT3puQU9JWURMM2wxZnZ3dGpQ?=
 =?utf-8?B?dWZrMzZrVTFWTWEzNHJIVXFjcktYVHFvbHo5VnE2Z2RpWnpma0JUelhkS08r?=
 =?utf-8?B?SVgzcmlaS2d5R0g1YUtkVVFtN2l0SFN5VkQwTXNkbUZEWFVUb1pvdGVxVUtz?=
 =?utf-8?B?aXNjQU91Z1BUVldPR3VwZlNpckE4R1NSM3pVQVBwaUUybWhORVlORThEMExV?=
 =?utf-8?B?V3dwbWhxUlI4QnpqcWZxeTNoNVNDVkVpOG5abC8wTVNxVHNNb0puamNvMGZL?=
 =?utf-8?B?NGxCTFM0cXdDOE1Tbm1oWFA2b3N0YjIzTW8rUm5xNjRML0J5ZTl4S0VwUHdq?=
 =?utf-8?B?YzdCeGNJMzFTU2RVTnlDNGRnVWV3aHJ5amlIWCs1R0RRR3prYnlxTkxaU0R6?=
 =?utf-8?B?NjhIbkFrdDNlbGgyUUErUXNDQkNpcFhHK1pZdmZnWnUxMnF5T205OUNYeEVI?=
 =?utf-8?B?SHdrUFU5WVpSdnlqQm1CLy8yQmhnS0drWktDQVM1emRMblMzaG1CSDVGbnA5?=
 =?utf-8?B?L3I5ZzJCRGx2YWhnSDUrQjBJc20xL0RXZGsyeEU4K3BmbFBKdVFLRHdOODhJ?=
 =?utf-8?B?Y2Fxd2s5eGRERmhJMG1tSWJjSVVGTVptcTBuU2V2TGg2emdsQ3I2ME4yKzVi?=
 =?utf-8?B?czRxYmYyN0tHTEhXUjVZYW5VclpGenlSSHhQRUhTejIyRjduem5xVDE0WWdY?=
 =?utf-8?B?RVMwK3ZxbVYxOHoydWhVdElUKzJIL0gyeENjcHJjWERUODNHRkFkRnpyTUFQ?=
 =?utf-8?B?aDNmS0Z6WG81OUFVcmJDcjYrYUE5eUxwQVFxeVhtMUhvTHpEeTdBS1Y2WFhs?=
 =?utf-8?B?czViek1FdDVqU0w3Zk83dVZtZW56ajd1cXZpbkFaUi9WVmx4aVY4Ykx1bmdH?=
 =?utf-8?B?OWNISWQwTWRtTzlOU24rSVFVUmdiS2lFN0kxVWFpVHgrTnFDWDJDMG5VNVpB?=
 =?utf-8?B?V0N3bkNhblVYakF4cm8xcTZ1WW9EMmNVWG4rU1YrV0VibzlaUDQ5V2dIUUFk?=
 =?utf-8?B?bGFGaThVVFJNSm44c0NsRU5ndVNVZEFnN2YzUForVTdoVW4yd1dNdXB0Sm52?=
 =?utf-8?B?TUVmdlI3aWNneGtEdkhRUzRwa2cvUkpsUWFiam12ckxQNWs2a01WZStOb0Zn?=
 =?utf-8?B?WFNSY0hrUW5WYkx2NS93QmRUdjRuYWd3V2l3a3g2VkVhUU5tanIzbnZOcW5t?=
 =?utf-8?Q?drg6BEK4uAC/yMhSi7zz07PRQt6vRPt3ZA?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 21955a05-4f65-4e6e-5df4-08db7ca0fdbf
X-MS-Exchange-CrossTenant-AuthSource: PH0PR10MB5433.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2023 15:11:49.9416
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cdevyW6vEU+ty/24nfl/DfxSq5vTkBsGwZTeqYQtaYm+R/UnRy/p0subqfkkYKInLiosCHdpKqCAOtzMe4C0zl6bPqkxWzI2olwge108/Ls=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB6964
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-04_09,2023-07-04_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0
 mlxlogscore=958 adultscore=0 bulkscore=0 suspectscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2305260000 definitions=main-2307040131
X-Proofpoint-ORIG-GUID: WVWOwTU6FwtZBPg0psNof9t4WgNtSHim
X-Proofpoint-GUID: WVWOwTU6FwtZBPg0psNof9t4WgNtSHim
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 7/4/23 15:37, Linus Torvalds wrote:
> On Tue, 4 Jul 2023 at 03:09, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>>
>> Commit 9b9879fc0327 added a hashtable storing lists of concurrent module
>> loads. However, it didn't fix up all the error paths in
>> init_module_from_file(); this would lead to leaving the function while an
>> on-stack 'struct idempotent' element is still in the hash table, which
>> leads to all sorts of badness as spotted by syzkaller:
> 
> You are of course 100% right.
> 
> However, I'd rather just use a wrapper function and make this thing
> much clearer. Like I should have done originally.
> 
> So I'd be inclined towards a patch like the attached instead. Works for you?

Looks mostly good. This bit is now included inside the concurrency check:

         if (!f || !(f->f_mode & FMODE_READ))
                 return -EBADF;

Since the cookie is file_inode(f) I think that means that you could have
one caller without FMODE_READ hit this check and it would potentially
return -EBADF even for other callers who did open the file properly.

Maybe just do the f_mode check in finit_module()? Or... new helper,
fdget_mode()??

Apart from this, there is another bit that looks a bit weird:

len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
if (len < 0) {
     mod_stat_inc(&failed_kreads);
     mod_stat_add_long(len, &invalid_kread_bytes);

I don't think we should be adding error codes to byte counts.


Vegard