This series of patches enables nfs rootfs support on the Morello board.
Patches 01 and 02 fix the inital kernel build error associated with a wrong function pointer type within the sunrpc modules due to the unlocked_ioctl fp, the error occurs upon enabling nfs within the defconfig.
Patches 03-09 deal with the fallout caused by changes inferred by patches 01 and 02. Details can be found in the description of patch 03.
Patch 10 is enabling nfs rootfs by default in the kernel.
It was confirmed that the kernel can boot with a nfs rootfs, the other affected modules were not tested.
Pawel Zalewski (10): net:sunrpc: fix incompatible function pointer type in cache net:sunrpc: fix incompatible function pointer type in rpc_pipe include:linux: proc_ioctl should take user_uintptr_t as argument net:sunrpc: fix incompatible function pointer type in cache sound:core: fix incompatible function pointer type in info scsi:esas2r: fix incompatible function pointer type in esas2r_main pci: fix incompatible function pointer type in proc hwmon: fix incompatible function pointer type in dell-smm-hwmon cpu:mtrr: fix incompatible function pointer type in if defconfig: enable nfs rootfs by default
arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ arch/x86/kernel/cpu/mtrr/if.c | 2 +- drivers/hwmon/dell-smm-hwmon.c | 2 +- drivers/pci/proc.c | 2 +- drivers/scsi/esas2r/esas2r_main.c | 2 +- include/linux/proc_fs.h | 4 ++-- include/sound/info.h | 2 +- net/sunrpc/cache.c | 6 +++--- net/sunrpc/rpc_pipe.c | 2 +- sound/core/info.c | 2 +- 10 files changed, 14 insertions(+), 12 deletions(-)
The function pointer file_operations.unlocked_ioctl requires the use of user_uintptr_t type as argument for CHERI capabilities so change the argument type from unsigned long to user_uintptr_t.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- net/sunrpc/cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index f075a9fb5ccc..c4aaef5430f1 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -991,7 +991,7 @@ static __poll_t cache_poll(struct file *filp, poll_table *wait, }
static int cache_ioctl(struct inode *ino, struct file *filp, - unsigned int cmd, unsigned long arg, + unsigned int cmd, user_uintptr_t arg, struct cache_detail *cd) { int len = 0; @@ -1791,7 +1791,7 @@ static __poll_t cache_poll_pipefs(struct file *filp, poll_table *wait) }
static long cache_ioctl_pipefs(struct file *filp, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { struct inode *inode = file_inode(filp); struct cache_detail *cd = RPC_I(inode)->private;
The function pointer file_operations.unlocked_ioctl requires the use of user_uintptr_t type as argument for CHERI capabilities so change the argument type from unsigned long to user_uintptr_t.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- net/sunrpc/rpc_pipe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index 0b6034fab9ab..1259e1e863f1 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -354,7 +354,7 @@ rpc_pipe_poll(struct file *filp, struct poll_table_struct *wait) }
static long -rpc_pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +rpc_pipe_ioctl(struct file *filp, unsigned int cmd, user_uintptr_t arg) { struct inode *inode = file_inode(filp); struct rpc_pipe *pipe;
The module net:sunrpc:cache.c will effectively use the same callback cache_ioctl for pointers passed from file_operations.unlocked_ioctl and proc_ops.proc_ioctl, the pointer passed from file_operations.unlocked_ioctl is of user_uintptr_t type, thus for consistency the argument in .proc_ioctl fp should also be of user_uintptr_t type.
Use user_uintptr_t in proc_fs for the argument instead of the unsigned long type.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- include/linux/proc_fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index 81d6e4ec2294..90ef95faf16d 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -36,9 +36,9 @@ struct proc_ops { loff_t (*proc_lseek)(struct file *, loff_t, int); int (*proc_release)(struct inode *, struct file *); __poll_t (*proc_poll)(struct file *, struct poll_table_struct *); - long (*proc_ioctl)(struct file *, unsigned int, unsigned long); + long (*proc_ioctl)(struct file *, unsigned int, user_uintptr_t); #ifdef CONFIG_COMPAT - long (*proc_compat_ioctl)(struct file *, unsigned int, unsigned long); + long (*proc_compat_ioctl)(struct file *, unsigned int, user_uintptr_t); #endif int (*proc_mmap)(struct file *, struct vm_area_struct *); unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
In the commit title: the description should be some sort of action, describing what the commit does. Using "should" doesn't make it clear the commit actually does that.
On 05/05/2023 17:36, Pawel Zalewski wrote:
The module net:sunrpc:cache.c will effectively use the same callback cache_ioctl for pointers passed from file_operations.unlocked_ioctl and proc_ops.proc_ioctl, the pointer passed from file_operations.unlocked_ioctl is of user_uintptr_t type, thus for consistency the argument in .proc_ioctl fp should also be of user_uintptr_t type.
This is not simply a matter of consistency: without using user_uintptr_t, we cannot propagate capabilities in PCuABI. Because at least one proc_ioctl handler (cache_ioctl_procfs) expects the argument to be a pointer, we need to change the callback signature (otherwise we would have been better off not changing it).
Use user_uintptr_t in proc_fs for the argument instead of the unsigned long type.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk
include/linux/proc_fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index 81d6e4ec2294..90ef95faf16d 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -36,9 +36,9 @@ struct proc_ops { loff_t (*proc_lseek)(struct file *, loff_t, int); int (*proc_release)(struct inode *, struct file *); __poll_t (*proc_poll)(struct file *, struct poll_table_struct *);
- long (*proc_ioctl)(struct file *, unsigned int, unsigned long);
- long (*proc_ioctl)(struct file *, unsigned int, user_uintptr_t);
#ifdef CONFIG_COMPAT
- long (*proc_compat_ioctl)(struct file *, unsigned int, unsigned long);
- long (*proc_compat_ioctl)(struct file *, unsigned int, user_uintptr_t);
We have avoided doing that for other compat ioctl callbacks, including the main file_operations one, because it is semantically incorrect: in compat(64), the argument really is just an unsigned long.
Instead of changing this callback signature, it would be better to fix the existing code that sets a proc_compat_ioctl callback. AFAICT we only need to fix proc_bus_pci_ops, and we can do it in the same way as esas2r_proc_ops, i.e. by using the compat_*_ioctl helpers. proc_bus_pci_ioctl() does not expect a pointer, so we should use the compat_noptr_ioctl helper there. See [1] for more information.
Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/next/Doc...
#endif int (*proc_mmap)(struct file *, struct vm_area_struct *); unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
In the commit title: the description should be some sort of action, describing what the commit does. Using "should" doesn't make it clear the commit actually does that.
ACK.
This is not simply a matter of consistency: without using
user_uintptr_t, we cannot propagate capabilities in PCuABI. Because at least one proc_ioctl handler (cache_ioctl_procfs) expects the argument to be a pointer, we need to change the callback signature (otherwise we would have been better off not changing it).
I am aware of this, the wording is poor indeed.
We have avoided doing that for other compat ioctl callbacks, including
the main file_operations one, because it is semantically incorrect: in compat(64), the argument really is just an unsigned long.
Instead of changing this callback signature, it would be better to fix the existing code that sets a proc_compat_ioctl callback. AFAICT we only need to fix proc_bus_pci_ops, and we can do it in the same way as esas2r_proc_ops, i.e. by using the compat_*_ioctl helpers. proc_bus_pci_ioctl() does not expect a pointer, so we should use the compat_noptr_ioctl helper there. See [1] for more information.
Noted.
Pawel
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- net/sunrpc/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index c4aaef5430f1..b682aa223bd6 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1573,7 +1573,7 @@ static __poll_t cache_poll_procfs(struct file *filp, poll_table *wait) }
static long cache_ioctl_procfs(struct file *filp, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { struct inode *inode = file_inode(filp); struct cache_detail *cd = pde_data(inode);
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- include/sound/info.h | 2 +- sound/core/info.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/sound/info.h b/include/sound/info.h index 7c13bf52cc81..b1853b950467 100644 --- a/include/sound/info.h +++ b/include/sound/info.h @@ -51,7 +51,7 @@ struct snd_info_entry_ops { void *file_private_data, struct file *file, poll_table *wait); int (*ioctl)(struct snd_info_entry *entry, void *file_private_data, - struct file *file, unsigned int cmd, unsigned long arg); + struct file *file, unsigned int cmd, user_uintptr_t arg); int (*mmap)(struct snd_info_entry *entry, void *file_private_data, struct inode *inode, struct file *file, struct vm_area_struct *vma); diff --git a/sound/core/info.c b/sound/core/info.c index 0b2f04dcb589..f1c579137d53 100644 --- a/sound/core/info.c +++ b/sound/core/info.c @@ -205,7 +205,7 @@ static __poll_t snd_info_entry_poll(struct file *file, poll_table *wait) }
static long snd_info_entry_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct snd_info_private_data *data = file->private_data; struct snd_info_entry *entry = data->entry;
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- drivers/scsi/esas2r/esas2r_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c index 7a4eadad23d7..d689a92c02fa 100644 --- a/drivers/scsi/esas2r/esas2r_main.c +++ b/drivers/scsi/esas2r/esas2r_main.c @@ -626,7 +626,7 @@ static const struct proc_ops esas2r_proc_ops = { static struct Scsi_Host *esas2r_proc_host; static int esas2r_proc_major;
-long esas2r_proc_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) +long esas2r_proc_ioctl(struct file *fp, unsigned int cmd, user_uintptr_t arg) { return esas2r_ioctl_handler(esas2r_proc_host->hostdata, cmd, (void __user *)arg);
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- drivers/pci/proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c index f967709082d6..3fe8993d8cc7 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -192,7 +192,7 @@ struct pci_filp_private { #endif /* HAVE_PCI_MMAP */
static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct pci_dev *dev = pde_data(file_inode(file)); #ifdef HAVE_PCI_MMAP
On 05/05/2023 17:36, Pawel Zalewski wrote:
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk
drivers/pci/proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c index f967709082d6..3fe8993d8cc7 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -192,7 +192,7 @@ struct pci_filp_private { #endif /* HAVE_PCI_MMAP */ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
user_uintptr_t arg)
Nit: some alignment whitespace has disappeared.
Kevin
{ struct pci_dev *dev = pde_data(file_inode(file)); #ifdef HAVE_PCI_MMAP
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- drivers/hwmon/dell-smm-hwmon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c index 1572b5416015..a83980aff963 100644 --- a/drivers/hwmon/dell-smm-hwmon.c +++ b/drivers/hwmon/dell-smm-hwmon.c @@ -432,7 +432,7 @@ static int i8k_get_power_status(void) * Procfs interface */
-static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) +static long i8k_ioctl(struct file *fp, unsigned int cmd, user_uintptr_t arg) { struct dell_smm_data *data = pde_data(file_inode(fp)); int __user *argp = (int __user *)arg;
The function pointer proc_ops.proc_ioctl requires the use of user_uintptr_t type as argument.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- arch/x86/kernel/cpu/mtrr/if.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c index a5c506f6da7f..4ffce21448fc 100644 --- a/arch/x86/kernel/cpu/mtrr/if.c +++ b/arch/x86/kernel/cpu/mtrr/if.c @@ -152,7 +152,7 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos) }
static long -mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg) +mtrr_ioctl(struct file *file, unsigned int cmd, user_uintptr_t __arg) { int err = 0; mtrr_type type;
It is very useful to be able to boot from a remote rootfs, especially during the development and due to the nature of the Morello project development will be the most often encountered use case.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk --- arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm64/configs/morello_transitional_pcuabi_defconfig b/arch/arm64/configs/morello_transitional_pcuabi_defconfig index 856806652bac..d1fefce6e727 100644 --- a/arch/arm64/configs/morello_transitional_pcuabi_defconfig +++ b/arch/arm64/configs/morello_transitional_pcuabi_defconfig @@ -147,6 +147,8 @@ CONFIG_TMPFS=y CONFIG_HUGETLBFS=y # CONFIG_EFIVAR_FS is not set CONFIG_9P_FS=y +CONFIG_NFS_FS=y +CONFIG_ROOT_NFS=y CONFIG_NLS_CODEPAGE_437=y CONFIG_NLS_ISO8859_1=y CONFIG_KEYS=y
In commit title: we use the "arm64: morello:" tag when updating this defconfig (see previous commits touching this file).
On 05/05/2023 17:36, Pawel Zalewski wrote:
It is very useful to be able to boot from a remote rootfs, especially during the development and due to the nature of the Morello project development will be the most often encountered use case.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk
arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/arm64/configs/morello_transitional_pcuabi_defconfig b/arch/arm64/configs/morello_transitional_pcuabi_defconfig index 856806652bac..d1fefce6e727 100644 --- a/arch/arm64/configs/morello_transitional_pcuabi_defconfig +++ b/arch/arm64/configs/morello_transitional_pcuabi_defconfig @@ -147,6 +147,8 @@ CONFIG_TMPFS=y CONFIG_HUGETLBFS=y # CONFIG_EFIVAR_FS is not set CONFIG_9P_FS=y +CONFIG_NFS_FS=y +CONFIG_ROOT_NFS=y
We try to update this defconfig according to the output of `make savedefconfig`, so that it remains stable and easy to diff. In that output, it looks like these two entries are just before CONFIG_9P_FS, not after.
Kevin
CONFIG_NLS_CODEPAGE_437=y CONFIG_NLS_ISO8859_1=y CONFIG_KEYS=y
In commit title: we use the "arm64: morello:" tag when updating this defconfig (see previous commits touching this file).
Noted.
We try to update this defconfig according to the output of `make
savedefconfig`, so that it remains stable and easy to diff. In that output, it looks like these two entries are just before CONFIG_9P_FS, not after.
Sure, I will address this in V2 as well.
Thanks for the feedback.
Pawel
On 05/05/2023 17:36, Pawel Zalewski wrote:
This series of patches enables nfs rootfs support on the Morello board.
Patches 01 and 02 fix the inital kernel build error associated with a wrong function pointer type within the sunrpc modules due to the unlocked_ioctl fp, the error occurs upon enabling nfs within the defconfig.
Patches 03-09 deal with the fallout caused by changes inferred by patches 01 and 02. Details can be found in the description of patch 03.
Patch 10 is enabling nfs rootfs by default in the kernel.
It was confirmed that the kernel can boot with a nfs rootfs, the other affected modules were not tested.
Pawel Zalewski (10): net:sunrpc: fix incompatible function pointer type in cache net:sunrpc: fix incompatible function pointer type in rpc_pipe include:linux: proc_ioctl should take user_uintptr_t as argument net:sunrpc: fix incompatible function pointer type in cache sound:core: fix incompatible function pointer type in info scsi:esas2r: fix incompatible function pointer type in esas2r_main pci: fix incompatible function pointer type in proc hwmon: fix incompatible function pointer type in dell-smm-hwmon cpu:mtrr: fix incompatible function pointer type in if defconfig: enable nfs rootfs by default
Thank you for your contribution, very appreciated! Overall the changes look sensible, using user_uintptr_t for ioctl handlers that expect pointers is the right thing to do.
Some general comments regarding commit messages: * Commit messages should be wrapped at 75 columns (or less), see [1]. * There should be a space between tags, e.g. "net: sunrpc:" rather than "net:sunrpc:". * File names are not normally mentioned in the commit title, however it's important to make it clear that it's about ioctl handlers. We could say for instance "fix unlocked_ioctl handler signature".
Regarding specific patches: * Patch 1-2 could be merged as they essentially do the same thing and target the same subsystem. * AFAICT we do not need the following patches because the corresponding subsystems are not part of the Morello defconfig (yet): - Patch 5 (sound is not enabled at all); - Patch 6 (CONFIG_SCSI_ESAS2R is not set); - Patch 8 (HW-specific file that is not relevant to Morello); - Patch 9 (x86 so not relevant to Morello). - Note: in general we avoid modifying files we do not build, as we are then unable to check whether the patch works at all. * Patch 3 will break the build, we try to avoid that for bisectability purposes. The easiest solution is to merge it with patch 4 and 7.
A few more specific comments follow in reply to some patches.
Kevin
[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-...
arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ arch/x86/kernel/cpu/mtrr/if.c | 2 +- drivers/hwmon/dell-smm-hwmon.c | 2 +- drivers/pci/proc.c | 2 +- drivers/scsi/esas2r/esas2r_main.c | 2 +- include/linux/proc_fs.h | 4 ++-- include/sound/info.h | 2 +- net/sunrpc/cache.c | 6 +++--- net/sunrpc/rpc_pipe.c | 2 +- sound/core/info.c | 2 +- 10 files changed, 14 insertions(+), 12 deletions(-)
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Pawel Zalewski