| Message ID | 20211029032638.84884-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | seq_file: fix passing wrong private data | expand |
On Fri, Oct 29, 2021 at 11:26:38AM +0800, Muchun Song wrote: > DEFINE_PROC_SHOW_ATTRIBUTE() is supposed to be used to define a series > of functions and variables to register proc file easily. And the users > can use proc_create_data() to pass their own private data and get it > via seq->private in the callback. Unfortunately, the proc file system > use PDE_DATA() to get private data instead of inode->i_private. So fix > it. Fortunately, there only one user of it which does not pass any > private data, so this bug does not break any in-tree codes. > > Fixes: 97a32539b956 ("proc: convert everything to "struct proc_ops"") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > include/linux/seq_file.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h > index 103776e18555..72dbb44a4573 100644 > --- a/include/linux/seq_file.h > +++ b/include/linux/seq_file.h > @@ -209,7 +209,7 @@ static const struct file_operations __name ## _fops = { \ > #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ > static int __name ## _open(struct inode *inode, struct file *file) \ > { \ > - return single_open(file, __name ## _show, inode->i_private); \ > + return single_open(file, __name ## _show, PDE_DATA(inode)); \ > } \ > \ > static const struct proc_ops __name ## _proc_ops = { \ Hm, after your change DEFINE_SHOW_ATTRIBUTE() and DEFINE_PROC_SHOW_ATTRIBUTE() macros do exactly the same things, right?: #define DEFINE_SHOW_ATTRIBUTE(__name) \ static int __name ## _open(struct inode *inode, struct file *file) \ { \ return single_open(file, __name ## _show, inode->i_private); \ } \ \ static const struct file_operations __name ## _fops = { \ .owner = THIS_MODULE, \ .open = __name ## _open, \ .read = seq_read, \ .llseek = seq_lseek, \ .release = single_release, \ } #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ static int __name ## _open(struct inode *inode, struct file *file) \ { \ return single_open(file, __name ## _show, inode->i_private); \ } \ Can't you just replace the single instance where DEFINE_PROC_SHOW_ATTRIBUTE with DEFINE_SHOW_ATTRIBUTE() and remove DEFINE_PROC_SHOW_ATTRIBUTE completely? Christian
On Fri, Oct 29, 2021 at 4:26 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Fri, Oct 29, 2021 at 11:26:38AM +0800, Muchun Song wrote: > > DEFINE_PROC_SHOW_ATTRIBUTE() is supposed to be used to define a series > > of functions and variables to register proc file easily. And the users > > can use proc_create_data() to pass their own private data and get it > > via seq->private in the callback. Unfortunately, the proc file system > > use PDE_DATA() to get private data instead of inode->i_private. So fix > > it. Fortunately, there only one user of it which does not pass any > > private data, so this bug does not break any in-tree codes. > > > > Fixes: 97a32539b956 ("proc: convert everything to "struct proc_ops"") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > include/linux/seq_file.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h > > index 103776e18555..72dbb44a4573 100644 > > --- a/include/linux/seq_file.h > > +++ b/include/linux/seq_file.h > > @@ -209,7 +209,7 @@ static const struct file_operations __name ## _fops = { \ > > #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ > > static int __name ## _open(struct inode *inode, struct file *file) \ > > { \ > > - return single_open(file, __name ## _show, inode->i_private); \ > > + return single_open(file, __name ## _show, PDE_DATA(inode)); \ > > } \ > > \ > > static const struct proc_ops __name ## _proc_ops = { \ > > Hm, after your change DEFINE_SHOW_ATTRIBUTE() and > DEFINE_PROC_SHOW_ATTRIBUTE() macros do exactly the same things, right?: Unfortunately, they are not the same. The difference is the operation structure, namely "struct file_operations" and "struct proc_ops". DEFINE_SHOW_ATTRIBUTE() is usually used by debugfs while DEFINE_SHOW_ATTRIBUTE() is used by procfs. Thanks. > > #define DEFINE_SHOW_ATTRIBUTE(__name) \ > static int __name ## _open(struct inode *inode, struct file *file) \ > { \ > return single_open(file, __name ## _show, inode->i_private); \ > } \ > \ > static const struct file_operations __name ## _fops = { \ > .owner = THIS_MODULE, \ > .open = __name ## _open, \ > .read = seq_read, \ > .llseek = seq_lseek, \ > .release = single_release, \ > } > > #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ > static int __name ## _open(struct inode *inode, struct file *file) \ > { \ > return single_open(file, __name ## _show, inode->i_private); \ > } \ > > Can't you just replace the single instance where > DEFINE_PROC_SHOW_ATTRIBUTE with DEFINE_SHOW_ATTRIBUTE() and remove > DEFINE_PROC_SHOW_ATTRIBUTE completely? > > Christian
On Fri, Oct 29, 2021 at 04:43:40PM +0800, Muchun Song wrote: > On Fri, Oct 29, 2021 at 4:26 PM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Fri, Oct 29, 2021 at 11:26:38AM +0800, Muchun Song wrote: > > > DEFINE_PROC_SHOW_ATTRIBUTE() is supposed to be used to define a series > > > of functions and variables to register proc file easily. And the users > > > can use proc_create_data() to pass their own private data and get it > > > via seq->private in the callback. Unfortunately, the proc file system > > > use PDE_DATA() to get private data instead of inode->i_private. So fix > > > it. Fortunately, there only one user of it which does not pass any > > > private data, so this bug does not break any in-tree codes. > > > > > > Fixes: 97a32539b956 ("proc: convert everything to "struct proc_ops"") > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > include/linux/seq_file.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h > > > index 103776e18555..72dbb44a4573 100644 > > > --- a/include/linux/seq_file.h > > > +++ b/include/linux/seq_file.h > > > @@ -209,7 +209,7 @@ static const struct file_operations __name ## _fops = { \ > > > #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ > > > static int __name ## _open(struct inode *inode, struct file *file) \ > > > { \ > > > - return single_open(file, __name ## _show, inode->i_private); \ > > > + return single_open(file, __name ## _show, PDE_DATA(inode)); \ > > > } \ > > > \ > > > static const struct proc_ops __name ## _proc_ops = { \ > > > > Hm, after your change DEFINE_SHOW_ATTRIBUTE() and > > DEFINE_PROC_SHOW_ATTRIBUTE() macros do exactly the same things, right?: > > Unfortunately, they are not the same. The difference is the > operation structure, namely "struct file_operations" and > "struct proc_ops". > > DEFINE_SHOW_ATTRIBUTE() is usually used by > debugfs while DEFINE_SHOW_ATTRIBUTE() is > used by procfs. Ugh, right, thanks for pointing that out. I overlooked the _proc_ops appendix. Not sure what's right here. There seem to have been earlier callers to DEFINE_PROC_SHOW_ATTRIBUTE() that relied on PDE_DATA() but there's only one caller so that change wouldn't be too bad, I guess.
On Fri, Oct 29, 2021 at 4:50 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Fri, Oct 29, 2021 at 04:43:40PM +0800, Muchun Song wrote: > > On Fri, Oct 29, 2021 at 4:26 PM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > > > > > > On Fri, Oct 29, 2021 at 11:26:38AM +0800, Muchun Song wrote: > > > > DEFINE_PROC_SHOW_ATTRIBUTE() is supposed to be used to define a series > > > > of functions and variables to register proc file easily. And the users > > > > can use proc_create_data() to pass their own private data and get it > > > > via seq->private in the callback. Unfortunately, the proc file system > > > > use PDE_DATA() to get private data instead of inode->i_private. So fix > > > > it. Fortunately, there only one user of it which does not pass any > > > > private data, so this bug does not break any in-tree codes. > > > > > > > > Fixes: 97a32539b956 ("proc: convert everything to "struct proc_ops"") > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > include/linux/seq_file.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h > > > > index 103776e18555..72dbb44a4573 100644 > > > > --- a/include/linux/seq_file.h > > > > +++ b/include/linux/seq_file.h > > > > @@ -209,7 +209,7 @@ static const struct file_operations __name ## _fops = { \ > > > > #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ > > > > static int __name ## _open(struct inode *inode, struct file *file) \ > > > > { \ > > > > - return single_open(file, __name ## _show, inode->i_private); \ > > > > + return single_open(file, __name ## _show, PDE_DATA(inode)); \ > > > > } \ > > > > \ > > > > static const struct proc_ops __name ## _proc_ops = { \ > > > > > > Hm, after your change DEFINE_SHOW_ATTRIBUTE() and > > > DEFINE_PROC_SHOW_ATTRIBUTE() macros do exactly the same things, right?: > > > > Unfortunately, they are not the same. The difference is the > > operation structure, namely "struct file_operations" and > > "struct proc_ops". > > > > DEFINE_SHOW_ATTRIBUTE() is usually used by > > debugfs while DEFINE_SHOW_ATTRIBUTE() is > > used by procfs. > > Ugh, right, thanks for pointing that out. I overlooked the _proc_ops > appendix. Not sure what's right here. There seem to have been earlier > callers to DEFINE_PROC_SHOW_ATTRIBUTE() that relied on PDE_DATA() but > there's only one caller so that change wouldn't be too bad, I guess. From my point of view, I think the macro is useful which can simplify the code. However there is only one user of it. I suppose few people know about it. For instance, the following ops can become the user of it. cifs_mount_params_proc_ops nfsd_proc_ops rpc_proc_ops Thanks.
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h index 103776e18555..72dbb44a4573 100644 --- a/include/linux/seq_file.h +++ b/include/linux/seq_file.h @@ -209,7 +209,7 @@ static const struct file_operations __name ## _fops = { \ #define DEFINE_PROC_SHOW_ATTRIBUTE(__name) \ static int __name ## _open(struct inode *inode, struct file *file) \ { \ - return single_open(file, __name ## _show, inode->i_private); \ + return single_open(file, __name ## _show, PDE_DATA(inode)); \ } \ \ static const struct proc_ops __name ## _proc_ops = { \
DEFINE_PROC_SHOW_ATTRIBUTE() is supposed to be used to define a series of functions and variables to register proc file easily. And the users can use proc_create_data() to pass their own private data and get it via seq->private in the callback. Unfortunately, the proc file system use PDE_DATA() to get private data instead of inode->i_private. So fix it. Fortunately, there only one user of it which does not pass any private data, so this bug does not break any in-tree codes. Fixes: 97a32539b956 ("proc: convert everything to "struct proc_ops"") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/seq_file.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)