Hi,
Thanks for looking at this.
On 7/23/25 4:50 AM, Catalin Marinas wrote:
On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
+/*
+ * Unlike put_user_gcs() above, the use of copy_from_user() may provide
+ * an opening for non GCS pages to be used to source data. Therefore this
+ * should only be used in contexts where that is acceptable.
+ */
Even in user space, the GCS pages can be read with normal loads, so
already usable as a data source if one wants to (not that it's of much
use). So not sure the comment here is needed.
Right, but userspace isn't using it in a privileged context to emulate
operations that have a permission check performed as part of the read
when performed by the HW.
This comment was added in V2 following a number of conversations about
whether this was an actual risk or something that is only a problem if a
long set of pre-conditions hold true. Conditions which can be summarized
as "it is too late anyway".
Hence the comment to remind people that this routine isn't assuring the
page is correctly marked.
I will reword it a bit if that is ok.
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().
+{
+ unsigned long ret;
+ u64 load = 0;
+
+ gcsb_dsync();
Might be worth a comment here, see the one in gcs_restore_signal().
Sure,
+ ret = copy_from_user(&load, addr, sizeof(load));
+ if (ret != 0)
+ *err = ret;
+ return load;
+}
Otherwise the patch looks fine:
Reviewed-by: Catalin Marinas <catalin.mari...@arm.com>
