| Message ID | 20230330085357.2653599-2-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI endpoint fixes and improvements | expand |
On Thu, Mar 30, 2023 at 05:53:41PM +0900, Damien Le Moal wrote: > A PCI endpoint function driver can define function specific attributes > under its function configfs directory using the add_cfs() endpoint > driver operation. This is done by tighing up the mkdir operation for > the function configfs directory to a call to the add_cfs() operation. > However, there are no checks preventing the user from repeatedly > creating function specific attribute directories with different names, > resulting in the same endpoint specific attributes group being added > multiple times, which also result in an invalid reference counting for > the attribute groups. E.g., using the pci-epf-ntb function driver as an > example, the user creates the function as follows: [...] > Fix this by modifying pci_epf_cfs_work() to execute the new function > pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs() > to obtain the function specific attribute group and the group name > (directory name) from the endpoint function driver. If the function > driver defines an attribute group, pci_ep_cfs_add_type_group() then > proceeds to register this group using configfs_register_group(), thus > automatically exposing the function type pecific onfigfs attributes to Still you haven't fixed this typo. But I don't expect you to respin unless there are other changes. > the user. E.g.: > > $ modprobe pci-epf-ntb > $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb > $ mkdir func0 > $ tree func0 > func0/ > |-- baseclass_code > |-- cache_line_size > |-- ... > |-- pci_epf_ntb.0 > | |-- db_count > | |-- mw1 > | |-- mw2 > | |-- mw3 > | |-- mw4 > | |-- num_mws > | `-- spad_count > |-- primary > |-- ... > `-- vendorid > > With this change, there is no need for the user to create or delete > directories in the endpoint function attributes directory. The > pci_epf_type_group_ops group operations are thus removed. > > The documentation for the pci-epf-ntb and pci-epf-vntb function drivers > are updated to reflect this change, removing the explanations showing > the need to manually create the sub-directory for the function specific > attributes. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> - Mani > --- > Documentation/PCI/endpoint/pci-ntb-howto.rst | 11 ++--- > Documentation/PCI/endpoint/pci-vntb-howto.rst | 13 +++--- > drivers/pci/endpoint/pci-ep-cfs.c | 41 +++++++++---------- > 3 files changed, 28 insertions(+), 37 deletions(-) > > diff --git a/Documentation/PCI/endpoint/pci-ntb-howto.rst b/Documentation/PCI/endpoint/pci-ntb-howto.rst > index 1884bf29caba..4261e7157ef1 100644 > --- a/Documentation/PCI/endpoint/pci-ntb-howto.rst > +++ b/Documentation/PCI/endpoint/pci-ntb-howto.rst > @@ -88,13 +88,10 @@ commands can be used:: > # echo 0x104c > functions/pci_epf_ntb/func1/vendorid > # echo 0xb00d > functions/pci_epf_ntb/func1/deviceid > > -In order to configure NTB specific attributes, a new sub-directory to func1 > -should be created:: > - > - # mkdir functions/pci_epf_ntb/func1/pci_epf_ntb.0/ > - > -The NTB function driver will populate this directory with various attributes > -that can be configured by the user:: > +The PCI endpoint framework also automatically creates a sub-directory in the > +function attribute directory. This sub-directory has the same name as the name > +of the function device and is populated with the following NTB specific > +attributes that can be configured by the user:: > > # ls functions/pci_epf_ntb/func1/pci_epf_ntb.0/ > db_count mw1 mw2 mw3 mw4 num_mws > diff --git a/Documentation/PCI/endpoint/pci-vntb-howto.rst b/Documentation/PCI/endpoint/pci-vntb-howto.rst > index 4ab8e4a26d4b..70d3bc90893f 100644 > --- a/Documentation/PCI/endpoint/pci-vntb-howto.rst > +++ b/Documentation/PCI/endpoint/pci-vntb-howto.rst > @@ -84,13 +84,10 @@ commands can be used:: > # echo 0x1957 > functions/pci_epf_vntb/func1/vendorid > # echo 0x0809 > functions/pci_epf_vntb/func1/deviceid > > -In order to configure NTB specific attributes, a new sub-directory to func1 > -should be created:: > - > - # mkdir functions/pci_epf_vntb/func1/pci_epf_vntb.0/ > - > -The NTB function driver will populate this directory with various attributes > -that can be configured by the user:: > +The PCI endpoint framework also automatically creates a sub-directory in the > +function attribute directory. This sub-directory has the same name as the name > +of the function device and is populated with the following NTB specific > +attributes that can be configured by the user:: > > # ls functions/pci_epf_vntb/func1/pci_epf_vntb.0/ > db_count mw1 mw2 mw3 mw4 num_mws > @@ -103,7 +100,7 @@ A sample configuration for NTB function is given below:: > # echo 1 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/num_mws > # echo 0x100000 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/mw1 > > -A sample configuration for virtual NTB driver for virutal PCI bus:: > +A sample configuration for virtual NTB driver for virtual PCI bus:: > > # echo 0x1957 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_vid > # echo 0x080A > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_pid > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > index 4b8ac0ac84d5..b16fc6093c20 100644 > --- a/drivers/pci/endpoint/pci-ep-cfs.c > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > @@ -23,6 +23,7 @@ struct pci_epf_group { > struct config_group group; > struct config_group primary_epc_group; > struct config_group secondary_epc_group; > + struct config_group *type_group; > struct delayed_work cfs_work; > struct pci_epf *epf; > int index; > @@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = { > .release = pci_epf_release, > }; > > -static struct config_group *pci_epf_type_make(struct config_group *group, > - const char *name) > -{ > - struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item); > - struct config_group *epf_type_group; > - > - epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group); > - return epf_type_group; > -} > - > -static void pci_epf_type_drop(struct config_group *group, > - struct config_item *item) > -{ > - config_item_put(item); > -} > - > -static struct configfs_group_operations pci_epf_type_group_ops = { > - .make_group = &pci_epf_type_make, > - .drop_item = &pci_epf_type_drop, > -}; > - > static const struct config_item_type pci_epf_type = { > - .ct_group_ops = &pci_epf_type_group_ops, > .ct_item_ops = &pci_epf_ops, > .ct_attrs = pci_epf_attrs, > .ct_owner = THIS_MODULE, > }; > > +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group) > +{ > + struct config_group *group; > + > + group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group); > + if (!group) > + return; > + > + if (IS_ERR(group)) { > + pr_err("failed to create epf type specific attributes\n"); > + return; > + } > + > + configfs_register_group(&epf_group->group, group); > +} > + > static void pci_epf_cfs_work(struct work_struct *work) > { > struct pci_epf_group *epf_group; > @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work) > pr_err("failed to create 'secondary' EPC interface\n"); > return; > } > + > + pci_ep_cfs_add_type_group(epf_group); > } > > static struct config_group *pci_epf_make(struct config_group *group, > -- > 2.39.2 >
On 3/31/23 14:22, Manivannan Sadhasivam wrote: > On Thu, Mar 30, 2023 at 05:53:41PM +0900, Damien Le Moal wrote: >> A PCI endpoint function driver can define function specific attributes >> under its function configfs directory using the add_cfs() endpoint >> driver operation. This is done by tighing up the mkdir operation for >> the function configfs directory to a call to the add_cfs() operation. >> However, there are no checks preventing the user from repeatedly >> creating function specific attribute directories with different names, >> resulting in the same endpoint specific attributes group being added >> multiple times, which also result in an invalid reference counting for >> the attribute groups. E.g., using the pci-epf-ntb function driver as an >> example, the user creates the function as follows: > > [...] > >> Fix this by modifying pci_epf_cfs_work() to execute the new function >> pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs() >> to obtain the function specific attribute group and the group name >> (directory name) from the endpoint function driver. If the function >> driver defines an attribute group, pci_ep_cfs_add_type_group() then >> proceeds to register this group using configfs_register_group(), thus >> automatically exposing the function type pecific onfigfs attributes to > > Still you haven't fixed this typo. But I don't expect you to respin unless there > are other changes. Arg. Sorry about that. I hope it can be fixed when applying.
On Thu, Mar 30, 2023 at 05:53:41PM +0900, Damien Le Moal wrote: > A PCI endpoint function driver can define function specific attributes > under its function configfs directory using the add_cfs() endpoint > driver operation. This is done by tighing up the mkdir operation for tightening? Or maybe "tying the mkdir ... to add_cfs()"? Not sure what you intended. > the function configfs directory to a call to the add_cfs() operation. > However, there are no checks preventing the user from repeatedly > creating function specific attribute directories with different names, > resulting in the same endpoint specific attributes group being added > multiple times, which also result in an invalid reference counting for > the attribute groups. E.g., using the pci-epf-ntb function driver as an > example, the user creates the function as follows: I don't know anything about configfs, but having the user mkdir things isn't the model I would have expected. IIUC this patch creates the subdirectory automatically without the user mkdir, so I have to wonder why it wasn't done that way in the first place. > $ modprobe pci-epf-ntb > $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb > $ mkdir func0 > $ tree func0 > func0/ > |-- baseclass_code > |-- cache_line_size > |-- ... > `-- vendorid > > $ mkdir func0/attrs > $ tree func0 > func0/ > |-- attrs > | |-- db_count > | |-- mw1 > | |-- mw2 > | |-- mw3 > | |-- mw4 > | |-- num_mws > | `-- spad_count > |-- baseclass_code > |-- cache_line_size > |-- ... > `-- vendorid > > At this point, the function can be started by linking the EP controller. > However, if the user mistakenly creates again a directory: > > $ mkdir func0/attrs2 > $ tree func0 > func0/ > |-- attrs > | |-- db_count > | |-- mw1 > | |-- mw2 > | |-- mw3 > | |-- mw4 > | |-- num_mws > | `-- spad_count > |-- attrs2 > | |-- db_count > | |-- mw1 > | |-- mw2 > | |-- mw3 > | |-- mw4 > | |-- num_mws > | `-- spad_count > |-- baseclass_code > |-- cache_line_size > |-- ... > `-- vendorid > > The function specific attributes are duplicated and cause a crash when > the function is tore down: torn > [ 9740.729598] ------------[ cut here ]------------ > [ 9740.730071] refcount_t: addition on 0; use-after-free. > [ 9740.730564] WARNING: CPU: 2 PID: 834 at lib/refcount.c:25 refcount_warn_saturate+0xc8/0x144 > [ 9740.735593] CPU: 2 PID: 834 Comm: rmdir Not tainted 6.3.0-rc1 #1 > [ 9740.736133] Hardware name: Pine64 RockPro64 v2.1 (DT) > [ 9740.736586] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 9740.737210] pc : refcount_warn_saturate+0xc8/0x144 > [ 9740.737648] lr : refcount_warn_saturate+0xc8/0x144 > [ 9740.738085] sp : ffff800009cebc90 > [ 9740.738385] x29: ffff800009cebc90 x28: ffff0000019ed700 x27: ffff0000040c3900 > [ 9740.739032] x26: 0000000000000000 x25: ffff800009325320 x24: ffff0000012da000 > [ 9740.739678] x23: ffff000003bd9a80 x22: ffff000005ee9580 x21: ffff000003bd9ad8 > [ 9740.740324] x20: ffff0000f36cd2c8 x19: ffff0000012da2b8 x18: 0000000000000006 > [ 9740.740969] x17: 0000000000000000 x16: 0000000000000000 x15: 0765076507720766 > [ 9740.741615] x14: 072d077207650774 x13: ffff800009281000 x12: 000000000000056d > [ 9740.742261] x11: 00000000000001cf x10: ffff8000092d9000 x9 : ffff800009281000 > [ 9740.742906] x8 : 00000000ffffefff x7 : ffff8000092d9000 x6 : 80000000fffff000 > [ 9740.743552] x5 : ffff0000f7771b88 x4 : 0000000000000000 x3 : 0000000000000027 > [ 9740.744197] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000019ed700 > [ 9740.744842] Call trace: > [ 9740.745068] refcount_warn_saturate+0xc8/0x144 > [ 9740.745475] config_item_get+0x7c/0x80 > [ 9740.745822] configfs_rmdir+0x17c/0x30c > [ 9740.746174] vfs_rmdir+0x8c/0x204 > [ 9740.746482] do_rmdir+0x158/0x184 > [ 9740.746787] __arm64_sys_unlinkat+0x64/0x80 > [ 9740.747171] invoke_syscall+0x48/0x114 > [ 9740.747519] el0_svc_common.constprop.0+0x44/0xec > [ 9740.747948] do_el0_svc+0x38/0x98 > [ 9740.748255] el0_svc+0x2c/0x84 > [ 9740.748541] el0t_64_sync_handler+0xf4/0x120 > [ 9740.748932] el0t_64_sync+0x190/0x194 > [ 9740.749269] ---[ end trace 0000000000000000 ]--- > [ 9740.749754] ------------[ cut here ]------------ "Cut here" lines, timestamps, and the bottom half of the call trace aren't really relevant here. Register contents probably not revelant either. > Fix this by modifying pci_epf_cfs_work() to execute the new function > pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs() > to obtain the function specific attribute group and the group name > (directory name) from the endpoint function driver. If the function > driver defines an attribute group, pci_ep_cfs_add_type_group() then > proceeds to register this group using configfs_register_group(), thus > automatically exposing the function type pecific onfigfs attributes to specific configfs? > the user. E.g.: > > $ modprobe pci-epf-ntb > $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb > $ mkdir func0 > $ tree func0 > func0/ > |-- baseclass_code > |-- cache_line_size > |-- ... > |-- pci_epf_ntb.0 > | |-- db_count > | |-- mw1 > | |-- mw2 > | |-- mw3 > | |-- mw4 > | |-- num_mws > | `-- spad_count > |-- primary > |-- ... > `-- vendorid > > With this change, there is no need for the user to create or delete > directories in the endpoint function attributes directory. The > pci_epf_type_group_ops group operations are thus removed. > > The documentation for the pci-epf-ntb and pci-epf-vntb function drivers > are updated to reflect this change, removing the explanations showing > the need to manually create the sub-directory for the function specific > attributes. "Update the documentation ... to reflect ..." (More direct than "The documentation ... is updated to reflect ...") > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > Documentation/PCI/endpoint/pci-ntb-howto.rst | 11 ++--- > Documentation/PCI/endpoint/pci-vntb-howto.rst | 13 +++--- > drivers/pci/endpoint/pci-ep-cfs.c | 41 +++++++++---------- > 3 files changed, 28 insertions(+), 37 deletions(-) > > diff --git a/Documentation/PCI/endpoint/pci-ntb-howto.rst b/Documentation/PCI/endpoint/pci-ntb-howto.rst > index 1884bf29caba..4261e7157ef1 100644 > --- a/Documentation/PCI/endpoint/pci-ntb-howto.rst > +++ b/Documentation/PCI/endpoint/pci-ntb-howto.rst > @@ -88,13 +88,10 @@ commands can be used:: > # echo 0x104c > functions/pci_epf_ntb/func1/vendorid > # echo 0xb00d > functions/pci_epf_ntb/func1/deviceid > > -In order to configure NTB specific attributes, a new sub-directory to func1 > -should be created:: > - > - # mkdir functions/pci_epf_ntb/func1/pci_epf_ntb.0/ > - > -The NTB function driver will populate this directory with various attributes > -that can be configured by the user:: > +The PCI endpoint framework also automatically creates a sub-directory in the > +function attribute directory. This sub-directory has the same name as the name > +of the function device and is populated with the following NTB specific > +attributes that can be configured by the user:: > > # ls functions/pci_epf_ntb/func1/pci_epf_ntb.0/ > db_count mw1 mw2 mw3 mw4 num_mws > diff --git a/Documentation/PCI/endpoint/pci-vntb-howto.rst b/Documentation/PCI/endpoint/pci-vntb-howto.rst > index 4ab8e4a26d4b..70d3bc90893f 100644 > --- a/Documentation/PCI/endpoint/pci-vntb-howto.rst > +++ b/Documentation/PCI/endpoint/pci-vntb-howto.rst > @@ -84,13 +84,10 @@ commands can be used:: > # echo 0x1957 > functions/pci_epf_vntb/func1/vendorid > # echo 0x0809 > functions/pci_epf_vntb/func1/deviceid > > -In order to configure NTB specific attributes, a new sub-directory to func1 > -should be created:: > - > - # mkdir functions/pci_epf_vntb/func1/pci_epf_vntb.0/ > - > -The NTB function driver will populate this directory with various attributes > -that can be configured by the user:: > +The PCI endpoint framework also automatically creates a sub-directory in the > +function attribute directory. This sub-directory has the same name as the name > +of the function device and is populated with the following NTB specific > +attributes that can be configured by the user:: > > # ls functions/pci_epf_vntb/func1/pci_epf_vntb.0/ > db_count mw1 mw2 mw3 mw4 num_mws > @@ -103,7 +100,7 @@ A sample configuration for NTB function is given below:: > # echo 1 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/num_mws > # echo 0x100000 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/mw1 > > -A sample configuration for virtual NTB driver for virutal PCI bus:: > +A sample configuration for virtual NTB driver for virtual PCI bus:: > > # echo 0x1957 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_vid > # echo 0x080A > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_pid > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > index 4b8ac0ac84d5..b16fc6093c20 100644 > --- a/drivers/pci/endpoint/pci-ep-cfs.c > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > @@ -23,6 +23,7 @@ struct pci_epf_group { > struct config_group group; > struct config_group primary_epc_group; > struct config_group secondary_epc_group; > + struct config_group *type_group; > struct delayed_work cfs_work; > struct pci_epf *epf; > int index; > @@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = { > .release = pci_epf_release, > }; > > -static struct config_group *pci_epf_type_make(struct config_group *group, > - const char *name) > -{ > - struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item); > - struct config_group *epf_type_group; > - > - epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group); > - return epf_type_group; > -} > - > -static void pci_epf_type_drop(struct config_group *group, > - struct config_item *item) > -{ > - config_item_put(item); > -} > - > -static struct configfs_group_operations pci_epf_type_group_ops = { > - .make_group = &pci_epf_type_make, > - .drop_item = &pci_epf_type_drop, > -}; > - > static const struct config_item_type pci_epf_type = { > - .ct_group_ops = &pci_epf_type_group_ops, > .ct_item_ops = &pci_epf_ops, > .ct_attrs = pci_epf_attrs, > .ct_owner = THIS_MODULE, > }; > > +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group) > +{ > + struct config_group *group; > + > + group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group); > + if (!group) > + return; > + > + if (IS_ERR(group)) { > + pr_err("failed to create epf type specific attributes\n"); Is it possible to connect this with a device via pci_err() or dev_err()? > + return; > + } > + > + configfs_register_group(&epf_group->group, group); > +} > + > static void pci_epf_cfs_work(struct work_struct *work) > { > struct pci_epf_group *epf_group; > @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work) > pr_err("failed to create 'secondary' EPC interface\n"); Same. > return; > } > + > + pci_ep_cfs_add_type_group(epf_group); > } > > static struct config_group *pci_epf_make(struct config_group *group, > -- > 2.39.2 >
On 4/15/23 00:58, Bjorn Helgaas wrote: [...] >> +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group) >> +{ >> + struct config_group *group; >> + >> + group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group); >> + if (!group) >> + return; >> + >> + if (IS_ERR(group)) { >> + pr_err("failed to create epf type specific attributes\n"); > > Is it possible to connect this with a device via pci_err() or > dev_err()? > >> + return; >> + } >> + >> + configfs_register_group(&epf_group->group, group); >> +} >> + >> static void pci_epf_cfs_work(struct work_struct *work) >> { >> struct pci_epf_group *epf_group; >> @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work) >> pr_err("failed to create 'secondary' EPC interface\n"); > > Same. I did not touch this line. It was there already. Another patch should change this. I can add another patch if you want... > >> return; >> } >> + >> + pci_ep_cfs_add_type_group(epf_group); >> } >> >> static struct config_group *pci_epf_make(struct config_group *group, >> -- >> 2.39.2 >>
diff --git a/Documentation/PCI/endpoint/pci-ntb-howto.rst b/Documentation/PCI/endpoint/pci-ntb-howto.rst index 1884bf29caba..4261e7157ef1 100644 --- a/Documentation/PCI/endpoint/pci-ntb-howto.rst +++ b/Documentation/PCI/endpoint/pci-ntb-howto.rst @@ -88,13 +88,10 @@ commands can be used:: # echo 0x104c > functions/pci_epf_ntb/func1/vendorid # echo 0xb00d > functions/pci_epf_ntb/func1/deviceid -In order to configure NTB specific attributes, a new sub-directory to func1 -should be created:: - - # mkdir functions/pci_epf_ntb/func1/pci_epf_ntb.0/ - -The NTB function driver will populate this directory with various attributes -that can be configured by the user:: +The PCI endpoint framework also automatically creates a sub-directory in the +function attribute directory. This sub-directory has the same name as the name +of the function device and is populated with the following NTB specific +attributes that can be configured by the user:: # ls functions/pci_epf_ntb/func1/pci_epf_ntb.0/ db_count mw1 mw2 mw3 mw4 num_mws diff --git a/Documentation/PCI/endpoint/pci-vntb-howto.rst b/Documentation/PCI/endpoint/pci-vntb-howto.rst index 4ab8e4a26d4b..70d3bc90893f 100644 --- a/Documentation/PCI/endpoint/pci-vntb-howto.rst +++ b/Documentation/PCI/endpoint/pci-vntb-howto.rst @@ -84,13 +84,10 @@ commands can be used:: # echo 0x1957 > functions/pci_epf_vntb/func1/vendorid # echo 0x0809 > functions/pci_epf_vntb/func1/deviceid -In order to configure NTB specific attributes, a new sub-directory to func1 -should be created:: - - # mkdir functions/pci_epf_vntb/func1/pci_epf_vntb.0/ - -The NTB function driver will populate this directory with various attributes -that can be configured by the user:: +The PCI endpoint framework also automatically creates a sub-directory in the +function attribute directory. This sub-directory has the same name as the name +of the function device and is populated with the following NTB specific +attributes that can be configured by the user:: # ls functions/pci_epf_vntb/func1/pci_epf_vntb.0/ db_count mw1 mw2 mw3 mw4 num_mws @@ -103,7 +100,7 @@ A sample configuration for NTB function is given below:: # echo 1 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/num_mws # echo 0x100000 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/mw1 -A sample configuration for virtual NTB driver for virutal PCI bus:: +A sample configuration for virtual NTB driver for virtual PCI bus:: # echo 0x1957 > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_vid # echo 0x080A > functions/pci_epf_vntb/func1/pci_epf_vntb.0/vntb_pid diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c index 4b8ac0ac84d5..b16fc6093c20 100644 --- a/drivers/pci/endpoint/pci-ep-cfs.c +++ b/drivers/pci/endpoint/pci-ep-cfs.c @@ -23,6 +23,7 @@ struct pci_epf_group { struct config_group group; struct config_group primary_epc_group; struct config_group secondary_epc_group; + struct config_group *type_group; struct delayed_work cfs_work; struct pci_epf *epf; int index; @@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = { .release = pci_epf_release, }; -static struct config_group *pci_epf_type_make(struct config_group *group, - const char *name) -{ - struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item); - struct config_group *epf_type_group; - - epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group); - return epf_type_group; -} - -static void pci_epf_type_drop(struct config_group *group, - struct config_item *item) -{ - config_item_put(item); -} - -static struct configfs_group_operations pci_epf_type_group_ops = { - .make_group = &pci_epf_type_make, - .drop_item = &pci_epf_type_drop, -}; - static const struct config_item_type pci_epf_type = { - .ct_group_ops = &pci_epf_type_group_ops, .ct_item_ops = &pci_epf_ops, .ct_attrs = pci_epf_attrs, .ct_owner = THIS_MODULE, }; +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group) +{ + struct config_group *group; + + group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group); + if (!group) + return; + + if (IS_ERR(group)) { + pr_err("failed to create epf type specific attributes\n"); + return; + } + + configfs_register_group(&epf_group->group, group); +} + static void pci_epf_cfs_work(struct work_struct *work) { struct pci_epf_group *epf_group; @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work) pr_err("failed to create 'secondary' EPC interface\n"); return; } + + pci_ep_cfs_add_type_group(epf_group); } static struct config_group *pci_epf_make(struct config_group *group,
A PCI endpoint function driver can define function specific attributes under its function configfs directory using the add_cfs() endpoint driver operation. This is done by tighing up the mkdir operation for the function configfs directory to a call to the add_cfs() operation. However, there are no checks preventing the user from repeatedly creating function specific attribute directories with different names, resulting in the same endpoint specific attributes group being added multiple times, which also result in an invalid reference counting for the attribute groups. E.g., using the pci-epf-ntb function driver as an example, the user creates the function as follows: $ modprobe pci-epf-ntb $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb $ mkdir func0 $ tree func0 func0/ |-- baseclass_code |-- cache_line_size |-- ... `-- vendorid $ mkdir func0/attrs $ tree func0 func0/ |-- attrs | |-- db_count | |-- mw1 | |-- mw2 | |-- mw3 | |-- mw4 | |-- num_mws | `-- spad_count |-- baseclass_code |-- cache_line_size |-- ... `-- vendorid At this point, the function can be started by linking the EP controller. However, if the user mistakenly creates again a directory: $ mkdir func0/attrs2 $ tree func0 func0/ |-- attrs | |-- db_count | |-- mw1 | |-- mw2 | |-- mw3 | |-- mw4 | |-- num_mws | `-- spad_count |-- attrs2 | |-- db_count | |-- mw1 | |-- mw2 | |-- mw3 | |-- mw4 | |-- num_mws | `-- spad_count |-- baseclass_code |-- cache_line_size |-- ... `-- vendorid The function specific attributes are duplicated and cause a crash when the function is tore down: [ 9740.729598] ------------[ cut here ]------------ [ 9740.730071] refcount_t: addition on 0; use-after-free. [ 9740.730564] WARNING: CPU: 2 PID: 834 at lib/refcount.c:25 refcount_warn_saturate+0xc8/0x144 [ 9740.735593] CPU: 2 PID: 834 Comm: rmdir Not tainted 6.3.0-rc1 #1 [ 9740.736133] Hardware name: Pine64 RockPro64 v2.1 (DT) [ 9740.736586] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 9740.737210] pc : refcount_warn_saturate+0xc8/0x144 [ 9740.737648] lr : refcount_warn_saturate+0xc8/0x144 [ 9740.738085] sp : ffff800009cebc90 [ 9740.738385] x29: ffff800009cebc90 x28: ffff0000019ed700 x27: ffff0000040c3900 [ 9740.739032] x26: 0000000000000000 x25: ffff800009325320 x24: ffff0000012da000 [ 9740.739678] x23: ffff000003bd9a80 x22: ffff000005ee9580 x21: ffff000003bd9ad8 [ 9740.740324] x20: ffff0000f36cd2c8 x19: ffff0000012da2b8 x18: 0000000000000006 [ 9740.740969] x17: 0000000000000000 x16: 0000000000000000 x15: 0765076507720766 [ 9740.741615] x14: 072d077207650774 x13: ffff800009281000 x12: 000000000000056d [ 9740.742261] x11: 00000000000001cf x10: ffff8000092d9000 x9 : ffff800009281000 [ 9740.742906] x8 : 00000000ffffefff x7 : ffff8000092d9000 x6 : 80000000fffff000 [ 9740.743552] x5 : ffff0000f7771b88 x4 : 0000000000000000 x3 : 0000000000000027 [ 9740.744197] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000019ed700 [ 9740.744842] Call trace: [ 9740.745068] refcount_warn_saturate+0xc8/0x144 [ 9740.745475] config_item_get+0x7c/0x80 [ 9740.745822] configfs_rmdir+0x17c/0x30c [ 9740.746174] vfs_rmdir+0x8c/0x204 [ 9740.746482] do_rmdir+0x158/0x184 [ 9740.746787] __arm64_sys_unlinkat+0x64/0x80 [ 9740.747171] invoke_syscall+0x48/0x114 [ 9740.747519] el0_svc_common.constprop.0+0x44/0xec [ 9740.747948] do_el0_svc+0x38/0x98 [ 9740.748255] el0_svc+0x2c/0x84 [ 9740.748541] el0t_64_sync_handler+0xf4/0x120 [ 9740.748932] el0t_64_sync+0x190/0x194 [ 9740.749269] ---[ end trace 0000000000000000 ]--- [ 9740.749754] ------------[ cut here ]------------ Fix this by modifying pci_epf_cfs_work() to execute the new function pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs() to obtain the function specific attribute group and the group name (directory name) from the endpoint function driver. If the function driver defines an attribute group, pci_ep_cfs_add_type_group() then proceeds to register this group using configfs_register_group(), thus automatically exposing the function type pecific onfigfs attributes to the user. E.g.: $ modprobe pci-epf-ntb $ cd /sys/kernel/config/pci_ep/functions/pci_epf_ntb $ mkdir func0 $ tree func0 func0/ |-- baseclass_code |-- cache_line_size |-- ... |-- pci_epf_ntb.0 | |-- db_count | |-- mw1 | |-- mw2 | |-- mw3 | |-- mw4 | |-- num_mws | `-- spad_count |-- primary |-- ... `-- vendorid With this change, there is no need for the user to create or delete directories in the endpoint function attributes directory. The pci_epf_type_group_ops group operations are thus removed. The documentation for the pci-epf-ntb and pci-epf-vntb function drivers are updated to reflect this change, removing the explanations showing the need to manually create the sub-directory for the function specific attributes. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- Documentation/PCI/endpoint/pci-ntb-howto.rst | 11 ++--- Documentation/PCI/endpoint/pci-vntb-howto.rst | 13 +++--- drivers/pci/endpoint/pci-ep-cfs.c | 41 +++++++++---------- 3 files changed, 28 insertions(+), 37 deletions(-)