From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E07E9EE645E for ; Thu, 12 Sep 2024 08:18:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0FD6E6B007B; Thu, 12 Sep 2024 04:18:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0AE3E6B0082; Thu, 12 Sep 2024 04:18:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EB7496B0083; Thu, 12 Sep 2024 04:18:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id CC5FB6B007B for ; Thu, 12 Sep 2024 04:18:58 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 77229C1E2D for ; Thu, 12 Sep 2024 08:18:58 +0000 (UTC) X-FDA: 82555385556.06.222746F Received: from mail-40131.protonmail.ch (mail-40131.protonmail.ch [185.70.40.131]) by imf29.hostedemail.com (Postfix) with ESMTP id 67A85120011 for ; Thu, 12 Sep 2024 08:18:56 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=proton.me header.s=rm764jmzjrfdlpu23jcgp3ybda.protonmail header.b=ZUS5ChCC; spf=pass (imf29.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1726129032; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=4BCBkUymeOe6HphWwcmNL8sLnaSR+aRCcfpXpcPxwcw=; b=7Ta3aH1aaqxKOu2aal0/6oRycU1JU9Km625rnqNb4NveRj2+yNVqCRv6FqSA+lfB+SNnhB 5AWtdj2mAXzuxwmdNWWiZIE3fN7IM2gfXXPWvvtKqgwcwpRX5JLemc1/AvbmDwBDLcocUh KskAry7cHJmgANssoU5271mZ8s68WCs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726129032; a=rsa-sha256; cv=none; b=Q3Nb7OVbj+Ioevt44iK6xWvRp3e5gxc7PgtLo4Q26G18h4T0vGpKptx8T9XnOYUPmMR8qj RmTE071L6LW/fxH6Vc8Yg/ydPB3m7/4wQ0L7F5BoXQkBrLuacLYPGZATNg8d8QDuU5VIcY vVQyGIxrpQUGLaYKGtLWbMNJfeBCwnk= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=proton.me header.s=rm764jmzjrfdlpu23jcgp3ybda.protonmail header.b=ZUS5ChCC; spf=pass (imf29.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=rm764jmzjrfdlpu23jcgp3ybda.protonmail; t=1726129133; x=1726388333; bh=4BCBkUymeOe6HphWwcmNL8sLnaSR+aRCcfpXpcPxwcw=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=ZUS5ChCC71JlgP0VafuBMNeaejGL/HBp/Pkfvrj5+IFLW8uLTnn+7Y+vdLWFYQaj1 5DM/MsgtJy6N4RMMPWe0cLWdBWfEMmuG/jusPJWzcoJxbJRvUdfxOeagrAtEqhhKk0 Xt3SWtlmA9CIbENxMX+U4QHVL91FvgXl0LUHURVOSlUWzjrpJHMqFIFbCUMML6Z7H4 XCm+ttKk6Wrt2HdhXd535JBtlDrVvD70KjmalaBmGAbwijpb2Hia9IvN4dZJqutclv AP1VlkFzhjv+sD5REWcxLByDqwiHFwy7RthhsbtNFW5NEBcnniEJfA1dOXnN0I2lco 3j2UT5uswxilA== Date: Thu, 12 Sep 2024 08:18:49 +0000 To: Danilo Krummrich From: Benno Lossin Cc: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, a.hindborg@samsung.com, aliceryhl@google.com, akpm@linux-foundation.org, daniel.almeida@collabora.com, faith.ekstrand@collabora.com, boris.brezillon@collabora.com, lina@asahilina.net, mcanal@igalia.com, zhiw@nvidia.com, cjia@nvidia.com, jhubbard@nvidia.com, airlied@redhat.com, ajanulgu@redhat.com, lyude@redhat.com, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v6 22/26] rust: alloc: implement `Cmalloc` in module allocator_test Message-ID: <9561eb0e-796c-4a39-b49a-00013052fe4b@proton.me> In-Reply-To: References: <20240816001216.26575-1-dakr@kernel.org> <20240816001216.26575-23-dakr@kernel.org> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 290574dee1d25111830bc8a59bafc078c55d9c5a MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 67A85120011 X-Stat-Signature: 3jyckop4naaf7pi3w9tyd3gnke8dzpmf X-HE-Tag: 1726129136-703557 X-HE-Meta: U2FsdGVkX18MgYG3++oyvis6Rkn9rjWXDK23A1OxQJ0HMXiXzZAcxGaNk2COZ8sHqkfuqQ5tOxZO8Pa7jWqf5giYFRYFWEpm80v4bjKU68wk7VuldR7/yhEZcXuTjnkd9tC2ZtB0omflKkTXgfw8Y5mOUEOyW6F6WN0GofBIDETlKf5sxcy+bQ81vEMpmkkznh+NUExKizWjDWjx3Nlk1v2P1Xlr+HpJ1DpVXBpgMaSoz6WXbQRlYHCQOo48Ghb3zvWADgOP/C6owT9FI84t006QKYd7gIPZDhZ/5QZyDnc7nK2YFJavV3wtzPT6K3VAh52Q48YJuT9giku3BEyobPR5BdwNsCbpPFOHTH6IQ3yoaz08YU6WKWQr/sesaZNq8XALktUt98REvW9UqPYWNn1qOaktiRr9ZH3WZPRaEN2f8SOBPsCzgrd/dTACqSRkyzZuRp967m5DLwr6OUBw5+nvnKFzPl8wHkqVRsis64RiuZlvoaCdq2Py31GxRoYhp+5B9Z1maSWvpwqnWsLGladV5x+7L128lbFwbOwMibEqQOvOCcWrK2bZ6I5Tz2wWXDmKelTsMdfI3bXV6id27tmvjoFuhUBqf+bGWiVN8OUEn61VMFrXE/UJosYGQpsXjqO66a8AAc3uMNx3VqPDmEoqcYxOhdV+AN5r840WAu8363DlWVkA3PvRhAIaxB0q0Wb07WxtxV24MbmJ0D8CL6QSg75BjJW/IaYnxJMVZX+6Yi1SL9MBjCqqqsp8dWjXDbGaR2k/zSieDScf3y0b8ZsKLiQFunquyd98jUN2NUg7yX03tNHLveEp79WNF+9kNrfUCIiM2bYEt8hbvcTxchmZVx1VgoJkHc5D+CH5fFl3ebfB1LJ0ZlAr1qJJY+bhfcokZ4ThJIyV/6JVOvZvjhvGZCfngIAzeZWwaf74sSDRtHwi4PYF+mWBUBbq90YIVms8+usSZQupFw6fyj8 a7izcaKn w3leYqv9GzjiKb/XdcefZTXrfpuRiooONOqK5I6vpofWn6KpAEY7K/0SFOhGg+2Ti1q1jqr15iDKjroXsRSfukmk7H/BLSYDNOT21arePTkTY1JBjj+rHoyAsuIsM8lbpZSFV3C5W24RoxDW9Ym6jk2KscPgCzTeYQxdp/IRNjAlcYJLqz7gSWKXnlOEreDnBNECzH3EJ3STgbrrtMESIp/+e5EmD967Z6VCZEeithinDSQOosLlyskMTjplX6F9WQoEjr4jE3AejfJTfcSH7+kVudeI4xmO/nE8QNvoHK0wJYK1SRrTcgOgDVGdOonL2WivQzF6MyWDMoeRpK/J0phgNNzx5otG7KGkCSYVuXIecSFezfl6MF3an4IIUIQrMl/EV439svMGFQzCnfy5JeCN4S61ZiqwSwNUfd4MIt34l/dSptH5g3FW/EV1zDsb9lDc3Zb6KJLpLBZUo84Te5pl8Pg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000067, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 11.09.24 16:37, Danilo Krummrich wrote: > On Wed, Sep 11, 2024 at 01:32:31PM +0000, Benno Lossin wrote: >> On 11.09.24 14:31, Danilo Krummrich wrote: >>> On Fri, Aug 30, 2024 at 12:25:27AM +0200, Danilo Krummrich wrote: >>>> On Thu, Aug 29, 2024 at 07:14:18PM +0000, Benno Lossin wrote: >>>>> On 16.08.24 02:11, Danilo Krummrich wrote: >>>>>> + >>>>>> + if layout.size() =3D=3D 0 { >>>>>> + // SAFETY: `src` has been created by `Self::alloc_store= _data`. >>>>> >>>>> This is not true, consider: >>>>> >>>>> let ptr =3D alloc(size =3D 0); >>>>> free(ptr) >>>>> >>>>> Alloc will return a dangling pointer due to the first if statement an= d >>>>> then this function will pass it to `free_read_data`, even though it >>>>> wasn't created by `alloc_store_data`. >>>>> This isn't forbidden by the `Allocator` trait function's safety >>>>> requirements. >>>>> >>>>>> + unsafe { Self::free_read_data(src) }; >>>>>> + >>>>>> + return Ok(NonNull::slice_from_raw_parts(NonNull::dangli= ng(), 0)); >>>>>> + } >>>>>> + >>>>>> + let dst =3D Self::alloc(layout, flags)?; >>>>>> + >>>>>> + // SAFETY: `src` has been created by `Self::alloc_store_dat= a`. >>>>>> + let data =3D unsafe { Self::data(src) }; >>>>> >>>>> Same issue here, if the allocation passed in is zero size. I think yo= u >>>>> have no other choice than to allocate even for zero size requests... >>>>> Otherwise how would you know that they are zero-sized. >>>> >>>> Good catch - gonna fix it. >>> >>> Almost got me. :) I think the code is fine, callers are not allowed to = pass >>> pointers to `realloc` and `free`, which haven't been allocated with the= same >>> corresponding allocator or are dangling. >> >> But what about the example above (ie the `alloc(size =3D 0)` and then >> `free`)? >=20 > This never has been valid for the `Allocator` trait. Look at `Kmalloc`, > `Vmalloc` and `KVmalloc`, they don't allow this either. That is true. > We've discussed this already in previous versions of this series, where f= or this > purpose, you asked for `old_layout` for `free`. Such that `free` can chec= k if > the `size` was zero and therefore return without doing anything. Yes, but that was only about the old_layout parameter (at least that's what I thought). >> I guess this all depends on how one interprets the term >> "existing, valid memory allocation". To me that describes anything an >> `Allocator` returns via `alloc` and `realloc`, including zero-sized >> allocations. >=20 > I argue that the dangling pointer returned for `size =3D=3D 0` does not p= oint to any > allocation in the sense of those allocators. It's just a dangling `[u8]` > pointer. Sure, but to me the concept of zero-sized allocations does exist. >> But if you argue that those are not valid allocations from that >> allocator, then that is not properly documented in the safety >> requirements of `Allocator`. >=20 > The safety requirements of `Allocator` where proposed by you and I though= t they > consider this aspect? No, they did not consider this aspect. I was under the impression, that we would still allow zero-sized allocations (in retrospect, this is stupid, since dangling pointers shouldn't be passed to `krealloc` etc.). > `realloc` has: >=20 > "If `ptr =3D=3D Some(p)`, then `p` must point to an existing and valid me= mory > allocation created by this allocator." >=20 > `free` has: >=20 > "`ptr` must point to an existing and valid memory allocation created by t= his > `Allocator` and must not be a dangling pointer." >=20 > We can add the part about the dangling pointer to `realloc` if you want. So I think we should do the following:=20 (1) Add a paragraph to the `Allocator` trait that explains that zero-sized allocations are not supported. (2) Add a check to `realloc` for zero-sized allocations + null pointer (ie a new allocation request) that prints a warning and returns an error (3) Instead of writing "existing and valid memory allocation created by this allocator", I think "valid non-zero-sized memory allocation created by this allocator" fits better. --- Cheers, Benno