OpenCL: Pinned memory fallbacks don't work

[magnumripper]

Some of our OpenCL formats try to allocate pinned memory for buffers with a fallback to normal buffers if the former fails. I always thought the code looked fishy so I looked a bit closer now and it's worse than I expected:

Current code (this from opencl_rawmd4_fmt_plug.c):

	pinned_saved_keys = clCreateBuffer(context[gpu_id], CL_MEM_READ_ONLY | CL_MEM_ALLOC_HOST_PTR, BUFSIZE * kpc, NULL, &ret_code);
	if (ret_code != CL_SUCCESS) {
		saved_plain = (cl_uint *) mem_alloc(BUFSIZE * kpc);
		if (saved_plain == NULL)
			HANDLE_CLERROR(ret_code, "Error creating page-locked memory pinned_saved_keys.");
	}
	else {
		saved_plain = (cl_uint *) clEnqueueMapBuffer(queue[gpu_id], pinned_saved_keys, CL_TRUE, CL_MAP_READ | CL_MAP_WRITE, 0, BUFSIZE * kpc, 0, NULL, NULL, &ret_code);
		HANDLE_CLERROR(ret_code, "Error mapping page-locked memory saved_plain.");
	}

(...)

	buffer_keys = clCreateBuffer(context[gpu_id], CL_MEM_READ_ONLY, BUFSIZE * kpc, NULL, &ret_code);

(...)

	BENCH_CLERROR(clEnqueueWriteBuffer(queue[gpu_id], buffer_keys, CL_TRUE, 0, 4 * key_idx, saved_plain, 0, NULL, NULL), "failed in clEnqueueWriteBuffer buffer_keys.");

1. I emulated a failure for the pinned alloc and sure enough the fallback just makes the format crash soon after.
2. The current code weirdly allocates twice the size needed: First a pinned buffer, then a normal one.
3. Even worse, the current code doesn't even use the pinned buffer ever: It uses that extra, non-pinned, one. Duh!

It's not hard to make it work as intended, the question is if it's even needed at all. I think not: When an allocation using CL_MEM_ALLOC_HOST_PTR fails to get pinned memory it will simply return non-pinned instead (which is why we never saw the mentioned crash). IMHO we should just drop that fallback code and start actually using the pinned buffer.

Something like this:

	device_buffer = clCreateBuffer(context[gpu_id], CL_MEM_READ_ONLY | CL_MEM_ALLOC_HOST_PTR, BUFSIZE * kpc, NULL, &ret_code);
	HANDLE_CLERROR(ret_code, "Error creating device buffer");
	host_buffer = clEnqueueMapBuffer(queue[gpu_id], device_buffer, CL_TRUE, CL_MAP_READ | CL_MAP_WRITE, 0, BUFSIZE * kpc, 0, NULL, NULL, &ret_code);
	HANDLE_CLERROR(ret_code, "Error mapping host buffer");

(...)

	BENCH_CLERROR(clEnqueueWriteBuffer(queue[gpu_id], device_buffer, CL_TRUE, 0, 4 * key_idx, host_buffer, 0, NULL, NULL), "failed in clEnqueueWriteBuffer buffer_keys.");

I authored some of the cases with b0rken code myself, blindly copied from existing formats.

[magnumripper]

This was an unexpected rabbit hole. It appears we've been doing this incorrectly for ages.

The actual alternatives are basically:
a) Do use clEnqueueWriteBuffer - but NOT with a mapped buffer.
b) Do map it, write to it, unmap. In this case we must not (and need not) use clEnqueueWriteBuffer. Data is transferred when we unmap.

But what we currently do is this unsupported mix: Map the buffer permanently to get a host pointer, write to it, then clEnqueueWriteBuffer before running kernel. This, according do ChatGPT 5, "is at best pointless (no speedup) and at worst incorrect (host-mapped region + device transfers concurrently)". Then again, I need to verify what the hallucinating oracle said.

[magnumripper]

But what we currently do is this unsupported mix: Map the buffer permanently to get a host pointer, write to it, then clEnqueueWriteBuffer before running kernel.

Oh, but we are rescued by this detail I wrote in OP:

3. Even worse, the current code doesn't even use the pinned buffer ever: It uses that extra, non-pinned, one. Duh!

Lulz. So in the end our current code isn't unsafe, it's basically just a very weird way to allocate a host side buffer. The worst effect of the permanent mapping is "it holds on to pinned/staging resources (...) until you unmap".

[magnumripper]

So in the end our current code isn't unsafe, it's basically just a very weird way to allocate a host side buffer

I've gone full circle now: Apparently our current code is correct (except for the mentioned fallbacks). The "map and never unmap" scheme we use is described by Nvidia - see page 10 of https://developer.download.nvidia.com/compute/DevZone/docs/html/OpenCL/doc/OpenCL_Best_Practices_Guide.pdf.

[magnumripper]

Fun fact: Hashcat doesn't specifically allocate pinned memory. There's no CL_MEM_ALLOC_HOST_PTR (nor CL_MEM_USE_HOST_PTR) at all, anywhere in their tree except for definitions of the macros. Maybe we're just over complicating it.