Received-SPF: pass (google.com: domain of linux-kernel+bounces-44874-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 30 Jan 2024 10:49:37 -0500
From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-kernel@vger.kernel.org>, Jose Souza <jose.souza@intel.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>, Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>, "Rafael J . Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 1/2] devcoredump: Remove devcoredump device if failing
 device is gone
Message-ID: <ZbkakWFtV5iQrfLP@intel.com>
References: <20240126151121.1076079-1-rodrigo.vivi@intel.com>
 <d57dc34fa8b0e25cec014b8001dcd0527d1c1013.camel@sipsolutions.net>
 <ZbgYyra1Ypa0nf6u@intel.com>
 <33df6c78c4c47a8f57a1c2bfe835065becb5a253.camel@sipsolutions.net>
 <ZbkSvcEtRgTXuzgJ@intel.com>
 <8681168464fa85061db4a7234f89cead65cb0261.camel@sipsolutions.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <8681168464fa85061db4a7234f89cead65cb0261.camel@sipsolutions.net>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?f48G/0ux5kFzMIpQK0n3I8YtUjx12bYVwWJsqBdtNqjdLdh4KxL3jLcELONh?=
 =?us-ascii?Q?H28KK5utPTkbd3CYidHb4lPb4HC0VF7KNVtORGKEyYfZlB8ACQXHoId7ap7d?=
 =?us-ascii?Q?AyNl/6HGgCovjmfFZ642zs1pI1voO28de1NPWD0/rPC+fkvZ9tBkRSTAs5fI?=
 =?us-ascii?Q?FsXdcSIc+E2kICo7+gXp1hASI+m2wxFpGXlgazeqQwH0fb7hPxqDSD+6WX8e?=
 =?us-ascii?Q?03u7F/OCJEOKqKw4idWJb4ztxuGpZTRDCBLAr4IEW5RmM8RD62hUEnOEXw47?=
 =?us-ascii?Q?PBmtpICefN6M7nvIZyCYROIVV2j1JyfVu54otej9k3O3oKj9rfP5TRWWwt0q?=
 =?us-ascii?Q?OguCRu8znxM03P4AS65sAzKK3R1eV+JakH32THFeOlzOX1Ip3uW6IShnBfvI?=
 =?us-ascii?Q?YFLYPaZ0BfTVhbO+4hN1Plv37n/uO5ar/aKNeqTMfLVuycKE57M1K5TJYVue?=
 =?us-ascii?Q?tNh6AwkI8LCSI7x0nZipot7f8I0C9qRVmzu5/PYUspvOWv6QMxNG8rYQufM6?=
 =?us-ascii?Q?6LeX8+tgzV4xk/5N9WmcHqEkmdmVy14RkQhxrUINup0xTsu8F7FJG4T/BH8W?=
 =?us-ascii?Q?o87AjEy1drjrHdzuw2vHXGbEU9bbCBAb1QbAItJPvbsd605AIorSq5qCXfjJ?=
 =?us-ascii?Q?2A8WuPv16cIvE4KoVxdiCUtZfTxMDpt552dcfeNcfPowuXVLSfPPK6xmPKiL?=
 =?us-ascii?Q?NuDkSpvONsz/9aS/pqdJUe/vlb8mL+UWlPpZMOqvW+xKRzYzIbCIachme+ee?=
 =?us-ascii?Q?TIwDPD5kmjDsoiRI/4+unALJxSzZEF6j30lVudy6KCNpex/kplSTBefReOIE?=
 =?us-ascii?Q?lGnRzIa7IOA7p9WRUnWruBlm4bPI70MTKicIkRHbl8jHYrHG+ojMDyMuxfEx?=
 =?us-ascii?Q?hRN3/3Hn42weO88nymjGWAcJtWO/sRcvLjKCoSyFoQfA2E3LbwffdFLTPyAf?=
 =?us-ascii?Q?4xBVrpsXziqL+YyGEKNoVGm1MXndTTPbf9go8wdBdzllz52sLDy9pksYic/e?=
 =?us-ascii?Q?KAU+n4gsxcdyQ8l2rm0cvA2AVFLAm4LucEdmks0YLs30WLIFt8ojXsuz6so6?=
 =?us-ascii?Q?q9ZgMCfsRInbykdbdFNqGEsUZD3xxXOScHUxlqGuNp8bKY2mCaqPsbGQnd8V?=
 =?us-ascii?Q?IwJo2hX49QBiotE4k6hpDMQ/0YsuNS7jFIva8+LbeFdhRqnFAzsYWw4EhQWd?=
 =?us-ascii?Q?G8O3gswHagsbfNoWMVQ6DAxgoDO7CoR8XxIDoXVJ/yPZcRjqb6Ct4xX5hCbC?=
 =?us-ascii?Q?AiElSrdJd/HHg1/LXRzdk8OmbWaN6C48LIxnofLc2bVvM6uBf3mjwRvb+Lud?=
 =?us-ascii?Q?03pbzOkfX1pHIcOH/Jgb0qFV4HNQ/P0Zckt9kYBlxHZf27Hfu5gSEQNsi6J3?=
 =?us-ascii?Q?fIfO6ZsYThRO6jwDUsHXA24L7+iBgkuA5XSiDcxhlsq+wh3N28+nNInQM/Y6?=
 =?us-ascii?Q?nsib8BzJTQfztiYbwd/wUV9cOSCW2BRJg6phA+KPY/9+v3NkMgoKvOJim/1s?=
 =?us-ascii?Q?LH3nKWj7uOEVltt/3I/XIvjKOn8pZM92XIX0zU6gaSS1p5ChBQG0cMdMAXC+?=
 =?us-ascii?Q?wZsCC9nxvfunZL05WRWUdjcNusZ/46j0OLimKXT4?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f1167c6-239d-4865-66b8-08dc21ab134b
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6059.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 15:49:43.0041
 (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: CpOr2t9IC7NEoso1CwS0UINi0hp4zQkyHtuMLueOsU7A9VNIwqUG15MiV+McGAD997Z00fnRyEuxz5vOWf3PoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7720
X-OriginatorOrg: intel.com

On Tue, Jan 30, 2024 at 04:19:18PM +0100, Johannes Berg wrote:
> On Tue, 2024-01-30 at 10:16 -0500, Rodrigo Vivi wrote:
> > > 
> > > But I'd rather not, it
> > > feels weird to have a need for it.
> > 
> > We could change or CI and instruct our devs to always write
> > something to 'data' to ensure that devcoredump is deleted
> > before we can reload our module. Maybe that's the right
> > approach indeed, although I would really prefer to have
> > a direct way.
> 
> That's not really what I meant :-) I think we can agree that it's wrong
> for the kernel to be _able_ to run into some kind of use-after-free if
> userspace isn't doing the right thing here!
> 
> What I meant though is: it's weird for 'data' to actually depend on the
> struct device being still around, no? Whatever you want 'data' to be,
> couldn't you arrange it so that it's valid as long as the module isn't
> removed, so that the 'data' pointer literally encapsulates the needed
> data, doesn't depend on anything else, and the method you pass is more
> like a 'format' method.

I'm sorry for not being clear here. I totally agree with you.

I will make changes to our driver to make the 'data' a standalone memory
that devcoredump will free. this ensures no uaf and no null deref.
data could be read even after unbinding the driver.

What I meant to userspace 'writing to 'data'' was to ensure that
on our CI we run something like

if /sys/.../device/devcd<n> exists, then
echo 1 > /sys/.../device/devcd<n>/data
before attempting the rmmod <driver>

our rmmod cannot get stuck or our CI is blocked, but then ensuring
the devcd is gone with module_put happening is the only current way
of not blocking the rmmod.

> 
> johannes