On 20/01/2023 15:39, Zahra Tarkhani wrote:
|The following is a small patch for enabling fuse file system. I tested it on my morello PC and FVP. branch: https://git.morello-project.org/zatkh/linux/-/tree/morello/fuse%7C
Hi Zahra,
Congratulations for posting your first Morello Linux patches! Overall the changes make sense to me. There are a few superficial issues though, I will go through them below (inline). If you haven't got the chance to look at it yet, the "Submitting patches" guide [1] is a very helpful overview of the kernel submission process.
To start with, you should make sure to post each patch as a separate email. I suspect you have already used git format-patch to generate a patch file for each commit, as well as a cover letter file (0000-*). You should add any relevant information about the patches in that cover letter, for instance the paragraph above with the link to your branch. Once you are happy with the cover letter and patches, pass git send-email *all the files* generated by git format-patch at once, and it will take care of the rest (sending one email per patch).
[1] https://docs.kernel.org/process/submitting-patches.html
|From 293351ba5a87beb4b20cc956025e5d7ab911a320 Mon Sep 17 00:00:00 2001 From: zatkh ztarkhani@microsoft.com|
It would be better to use your full name as author name, you can change that using e.g. git config --global user.name "..." and then git commit --amend --reset-author.
|Date: Wed, 11 Jan 2023 16:39:07 +0000 Subject: [PATCH 1/2] kernel fusefs enable|
See the section "The canonical patch format" in [1] about what the commit summary + message should contain. I would recommend looking at the history of the files you are touching for inspiration, especially to choose the tag in the commit summary.
The Signed-off-by tag is especially important, you can generate it automatically using git commit -s (section "Developer’s Certificate of Origin 1.1" in [1]).
|--- arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ fs/fuse/file.c | 2 +- fs/fuse/fuse_i.h | 2 +-|
This certainly varies a lot between projects, but for what concerns Linux, there is a strong preference to have series of small patches. In particular, if at all possible, modifying two completely different areas of the kernel in the same patch should be avoided, especially if one is generic (here fs/*) and the other arch code (arch/arm64/*).
Since there is no requirement to enable the option in defconfig before modifying fs/*, the best would be to have one patch that fixes the issues under fs/fuse/, then another patch that adds the option to the defconfig. It would be perfectly appropriate to add both CONFIG_FUSE_FS and CONFIG_CUSE in the same patch (at the end of the series), though having two separate patches is just fine as well.
|3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/arm64/configs/morello_transitional_pcuabi_defconfig b/arch/arm64/configs/morello_transitional_pcuabi_defconfig index 2c0798c18553..af37f9ed4e36 100644 --- a/arch/arm64/configs/morello_transitional_pcuabi_defconfig +++ b/arch/arm64/configs/morello_transitional_pcuabi_defconfig @@ -130,6 +130,7 @@ CONFIG_VFAT_FS=y CONFIG_TMPFS=y CONFIG_HUGETLBFS=y CONFIG_CONFIGFS_FS=y +CONFIG_FUSE_FS=y|
Not a big issue but we are trying to keep this defconfig file consistent with the output of savedefconfig, to keep it minimal and easy to diff. This means that you should update the file by replacing it with the output of savedefconfig, something along these lines:
make morello_transitional_pcuabi_defconfig <edit .config using e.g. make menuconfig> make savedefconfig cp defconfig arch/arm64/configs/morello_transitional_pcuabi_defconfig <check git diff>
|# CONFIG_EFIVAR_FS is not set CONFIG_9P_FS=y CONFIG_NLS_CODEPAGE_437=y @@ -147,3 +148,4 @@ CONFIG_DEBUG_FS=y # CONFIG_DEBUG_PREEMPT is not set # CONFIG_FTRACE is not set CONFIG_MEMTEST=y +|
Again not a big problem but please make sure to avoid unnecessary changes like this one.
|diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 2318e66e7612..71b55bb83d25 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1366,7 +1366,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from) static inline unsigned long fuse_get_user_addr(const struct iov_iter *ii) { - return (unsigned long)ii->iov->iov_base + ii->iov_offset; + return (__cheri_addr long)ii->iov->iov_base + ii->iov_offset;|
I've tried to look into this code a little bit closer and I believe this is actually an upstream bug - there is no need to extract the address out of a user pointer here.
Doing a bit of archaeology, this code was introduced in 2012 by 7c190c8b9c0d ("fuse: optimize fuse_get_user_pages()"). That was before a280455fa870 ("iov_iter.c: handle ITER_KVEC directly") added a kvec member to struct iov_iter, which would explain why this function is using ii->iov and not ii->kvec.
In any case, I think replacing ii->iov with ii->kvec (and leaving the cast unchanged) is the right thing to do here, as we know that iov_iter_is_kvec(ii) holds. I believe it would be a very good thing to upstream this patch as it's a clear bugfix, though it's of course entirely up to you to do it or not.
Take your time to go through my comments. When you have a new version ready, use git format-patch -v2 to add "v2" to the prefix of each patch, and you can then send them as I suggested at the beginning.
Cheers, Kevin
|} static inline size_t fuse_get_frag_size(const struct iov_iter *ii, diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 081fd8d0b45d..005801a8c07b 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1302,7 +1302,7 @@ bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment); void fuse_dax_cancel_work(struct fuse_conn *fc); /* ioctl.c */ -long fuse_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg); +long fuse_file_ioctl(struct file *file, unsigned int cmd, user_uintptr_t arg); long fuse_file_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg); int fuse_fileattr_get(struct dentry *dentry, struct fileattr *fa); -- 2.25.1 From d5af2386cd9802c77c3682fcc7459e4abe060737 Mon Sep 17 00:00:00 2001 From: ztarkhani ztarkhani@microsoft.com Date: Fri, 13 Jan 2023 12:17:11 +0000 Subject: [PATCH 2/2] enable CUSE --- arch/arm64/configs/morello_transitional_pcuabi_defconfig | 1 + fs/fuse/cuse.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/configs/morello_transitional_pcuabi_defconfig b/arch/arm64/configs/morello_transitional_pcuabi_defconfig index af37f9ed4e36..829432db9807 100644 --- a/arch/arm64/configs/morello_transitional_pcuabi_defconfig +++ b/arch/arm64/configs/morello_transitional_pcuabi_defconfig @@ -131,6 +131,7 @@ CONFIG_TMPFS=y CONFIG_HUGETLBFS=y CONFIG_CONFIGFS_FS=y CONFIG_FUSE_FS=y +CONFIG_CUSE=y # CONFIG_EFIVAR_FS is not set CONFIG_9P_FS=y CONFIG_NLS_CODEPAGE_437=y diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c index c7d882a9fe33..5dc123f903a9 100644 --- a/fs/fuse/cuse.c +++ b/fs/fuse/cuse.c @@ -153,7 +153,7 @@ static int cuse_release(struct inode *inode, struct file *file) } static long cuse_file_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct fuse_file *ff = file->private_data; struct cuse_conn *cc = fc_to_cc(ff->fm->fc); -- 2.25.1 ____________|