| Message ID | 20220701132826.8132-2-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ifcvf/vDPA: support query device config space through netlink | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: > > ifcvf_get_config_size() should return a virtio device type specific value, > however the ret_value should not be greater than the onboard size of > the device implementation. E.g., for virtio_net, config_size should be > the minimum value of sizeof(struct virtio_net_config) and the onboard > cap size. Rethink of this, I wonder what's the value of exposing device implementation details to users? Anyhow the parent is in charge of "emulating" config space accessing. If we do this, it's probably a blocker for cross vendor stuff. Thanks > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- > drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > index 48c4dadb0c7c..fb957b57941e 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.c > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) > break; > case VIRTIO_PCI_CAP_DEVICE_CFG: > hw->dev_cfg = get_cap_addr(hw, &cap); > + hw->cap_dev_config_size = le32_to_cpu(cap.length); > IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); > break; > } > @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) > u32 ifcvf_get_config_size(struct ifcvf_hw *hw) > { > struct ifcvf_adapter *adapter; > + u32 net_config_size = sizeof(struct virtio_net_config); > + u32 blk_config_size = sizeof(struct virtio_blk_config); > + u32 cap_size = hw->cap_dev_config_size; > u32 config_size; > > adapter = vf_to_adapter(hw); > + /* If the onboard device config space size is greater than > + * the size of struct virtio_net/blk_config, only the spec > + * implementing contents size is returned, this is very > + * unlikely, defensive programming. > + */ > switch (hw->dev_type) { > case VIRTIO_ID_NET: > - config_size = sizeof(struct virtio_net_config); > + config_size = cap_size >= net_config_size ? net_config_size : cap_size; > break; > case VIRTIO_ID_BLOCK: > - config_size = sizeof(struct virtio_blk_config); > + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; > break; > default: > config_size = 0; > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > index 115b61f4924b..f5563f665cc6 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > @@ -87,6 +87,8 @@ struct ifcvf_hw { > int config_irq; > int vqs_reused_irq; > u16 nr_vring; > + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ > + u32 cap_dev_config_size; > }; > > struct ifcvf_adapter { > -- > 2.31.1 >
On 7/4/2022 12:39 PM, Jason Wang wrote: > On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: >> ifcvf_get_config_size() should return a virtio device type specific value, >> however the ret_value should not be greater than the onboard size of >> the device implementation. E.g., for virtio_net, config_size should be >> the minimum value of sizeof(struct virtio_net_config) and the onboard >> cap size. > Rethink of this, I wonder what's the value of exposing device > implementation details to users? Anyhow the parent is in charge of > "emulating" config space accessing. This will not be exposed to the users, it is a ifcvf internal helper, to get the actual device config space size. For example, if ifcvf drives an Intel virtio-net device, if the device config space size is greater than sizeof(struct virtio_net_cfg), this means the device has something more than the spec, some private fields, we don't want to expose these extra private fields to the users, so in this case, we only return what the spec defines. If the device config space size is less than sizeof(struct virtio_net_cfg), means the device didn't implement all fields the spec defined, like no RSS. In such cases, we only return what the device implemented. So these are defensive programming. > > If we do this, it's probably a blocker for cross vendor stuff. > > Thanks > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- >> drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c >> index 48c4dadb0c7c..fb957b57941e 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >> @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) >> break; >> case VIRTIO_PCI_CAP_DEVICE_CFG: >> hw->dev_cfg = get_cap_addr(hw, &cap); >> + hw->cap_dev_config_size = le32_to_cpu(cap.length); >> IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); >> break; >> } >> @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) >> u32 ifcvf_get_config_size(struct ifcvf_hw *hw) >> { >> struct ifcvf_adapter *adapter; >> + u32 net_config_size = sizeof(struct virtio_net_config); >> + u32 blk_config_size = sizeof(struct virtio_blk_config); >> + u32 cap_size = hw->cap_dev_config_size; >> u32 config_size; >> >> adapter = vf_to_adapter(hw); >> + /* If the onboard device config space size is greater than >> + * the size of struct virtio_net/blk_config, only the spec >> + * implementing contents size is returned, this is very >> + * unlikely, defensive programming. >> + */ >> switch (hw->dev_type) { >> case VIRTIO_ID_NET: >> - config_size = sizeof(struct virtio_net_config); >> + config_size = cap_size >= net_config_size ? net_config_size : cap_size; >> break; >> case VIRTIO_ID_BLOCK: >> - config_size = sizeof(struct virtio_blk_config); >> + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; >> break; >> default: >> config_size = 0; >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h >> index 115b61f4924b..f5563f665cc6 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >> @@ -87,6 +87,8 @@ struct ifcvf_hw { >> int config_irq; >> int vqs_reused_irq; >> u16 nr_vring; >> + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ >> + u32 cap_dev_config_size; >> }; >> >> struct ifcvf_adapter { >> -- >> 2.31.1 >>
On Fri, Jul 01, 2022 at 09:28:21PM +0800, Zhu Lingshan wrote: > ifcvf_get_config_size() should return a virtio device type specific value, > however the ret_value should not be greater than the onboard size of > the device implementation. E.g., for virtio_net, config_size should be > the minimum value of sizeof(struct virtio_net_config) and the onboard > cap size. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- > drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > index 48c4dadb0c7c..fb957b57941e 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.c > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) > break; > case VIRTIO_PCI_CAP_DEVICE_CFG: > hw->dev_cfg = get_cap_addr(hw, &cap); > + hw->cap_dev_config_size = le32_to_cpu(cap.length); > IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); > break; > } > @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) > u32 ifcvf_get_config_size(struct ifcvf_hw *hw) > { > struct ifcvf_adapter *adapter; > + u32 net_config_size = sizeof(struct virtio_net_config); > + u32 blk_config_size = sizeof(struct virtio_blk_config); > + u32 cap_size = hw->cap_dev_config_size; > u32 config_size; > > adapter = vf_to_adapter(hw); > + /* If the onboard device config space size is greater than > + * the size of struct virtio_net/blk_config, only the spec > + * implementing contents size is returned, this is very > + * unlikely, defensive programming. > + */ > switch (hw->dev_type) { > case VIRTIO_ID_NET: > - config_size = sizeof(struct virtio_net_config); > + config_size = cap_size >= net_config_size ? net_config_size : cap_size; > break; > case VIRTIO_ID_BLOCK: > - config_size = sizeof(struct virtio_blk_config); > + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; > break; > default: > config_size = 0; There's a min macro for this. > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > index 115b61f4924b..f5563f665cc6 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > @@ -87,6 +87,8 @@ struct ifcvf_hw { > int config_irq; > int vqs_reused_irq; > u16 nr_vring; > + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ > + u32 cap_dev_config_size; > }; > > struct ifcvf_adapter { > -- > 2.31.1
On Fri, Jul 08, 2022 at 02:44:08PM +0800, Zhu, Lingshan wrote: > > > On 7/4/2022 12:39 PM, Jason Wang wrote: > > On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: > > > ifcvf_get_config_size() should return a virtio device type specific value, > > > however the ret_value should not be greater than the onboard size of > > > the device implementation. E.g., for virtio_net, config_size should be > > > the minimum value of sizeof(struct virtio_net_config) and the onboard > > > cap size. > > Rethink of this, I wonder what's the value of exposing device > > implementation details to users? Anyhow the parent is in charge of > > "emulating" config space accessing. > This will not be exposed to the users, it is a ifcvf internal helper, > to get the actual device config space size. > > For example, if ifcvf drives an Intel virtio-net device, > if the device config space size is greater than sizeof(struct > virtio_net_cfg), > this means the device has something more than the spec, some private fields, > we don't want to expose these extra private fields to the users, so in this > case, > we only return what the spec defines. This is kind of already the case. > If the device config space size is less than sizeof(struct virtio_net_cfg), > means the device didn't implement all fields the spec defined, like no RSS. > In such cases, we only return what the device implemented. > So these are defensive programming. I think the issue you are describing is simply this. Driver must not access BAR outside capability length. Current code does not verify that it does not. Not the case for the current devices but it's best to be safe against the case where device does not implement some of the capability. From that POV I think the patch is good, just fix the log. > > > > If we do this, it's probably a blocker for cross vendor stuff. > > > > Thanks > > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > > > --- > > > drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- > > > drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ > > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > > > index 48c4dadb0c7c..fb957b57941e 100644 > > > --- a/drivers/vdpa/ifcvf/ifcvf_base.c > > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > > > @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) > > > break; > > > case VIRTIO_PCI_CAP_DEVICE_CFG: > > > hw->dev_cfg = get_cap_addr(hw, &cap); > > > + hw->cap_dev_config_size = le32_to_cpu(cap.length); > > > IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); > > > break; > > > } > > > @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) > > > u32 ifcvf_get_config_size(struct ifcvf_hw *hw) > > > { > > > struct ifcvf_adapter *adapter; > > > + u32 net_config_size = sizeof(struct virtio_net_config); > > > + u32 blk_config_size = sizeof(struct virtio_blk_config); > > > + u32 cap_size = hw->cap_dev_config_size; > > > u32 config_size; > > > > > > adapter = vf_to_adapter(hw); > > > + /* If the onboard device config space size is greater than > > > + * the size of struct virtio_net/blk_config, only the spec > > > + * implementing contents size is returned, this is very > > > + * unlikely, defensive programming. > > > + */ > > > switch (hw->dev_type) { > > > case VIRTIO_ID_NET: > > > - config_size = sizeof(struct virtio_net_config); > > > + config_size = cap_size >= net_config_size ? net_config_size : cap_size; > > > break; > > > case VIRTIO_ID_BLOCK: > > > - config_size = sizeof(struct virtio_blk_config); > > > + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; > > > break; > > > default: > > > config_size = 0; > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > > > index 115b61f4924b..f5563f665cc6 100644 > > > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > > > @@ -87,6 +87,8 @@ struct ifcvf_hw { > > > int config_irq; > > > int vqs_reused_irq; > > > u16 nr_vring; > > > + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ > > > + u32 cap_dev_config_size; > > > }; > > > > > > struct ifcvf_adapter { > > > -- > > > 2.31.1 > > >
On 7/13/2022 1:31 PM, Michael S. Tsirkin wrote: > On Fri, Jul 01, 2022 at 09:28:21PM +0800, Zhu Lingshan wrote: >> ifcvf_get_config_size() should return a virtio device type specific value, >> however the ret_value should not be greater than the onboard size of >> the device implementation. E.g., for virtio_net, config_size should be >> the minimum value of sizeof(struct virtio_net_config) and the onboard >> cap size. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- >> drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c >> index 48c4dadb0c7c..fb957b57941e 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >> @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) >> break; >> case VIRTIO_PCI_CAP_DEVICE_CFG: >> hw->dev_cfg = get_cap_addr(hw, &cap); >> + hw->cap_dev_config_size = le32_to_cpu(cap.length); >> IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); >> break; >> } >> @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) >> u32 ifcvf_get_config_size(struct ifcvf_hw *hw) >> { >> struct ifcvf_adapter *adapter; >> + u32 net_config_size = sizeof(struct virtio_net_config); >> + u32 blk_config_size = sizeof(struct virtio_blk_config); >> + u32 cap_size = hw->cap_dev_config_size; >> u32 config_size; >> >> adapter = vf_to_adapter(hw); >> + /* If the onboard device config space size is greater than >> + * the size of struct virtio_net/blk_config, only the spec >> + * implementing contents size is returned, this is very >> + * unlikely, defensive programming. >> + */ >> switch (hw->dev_type) { >> case VIRTIO_ID_NET: >> - config_size = sizeof(struct virtio_net_config); >> + config_size = cap_size >= net_config_size ? net_config_size : cap_size; >> break; >> case VIRTIO_ID_BLOCK: >> - config_size = sizeof(struct virtio_blk_config); >> + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; >> break; >> default: >> config_size = 0; > There's a min macro for this. yes, a min macro is better. Thanks, Zhu Lingshan > >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h >> index 115b61f4924b..f5563f665cc6 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >> @@ -87,6 +87,8 @@ struct ifcvf_hw { >> int config_irq; >> int vqs_reused_irq; >> u16 nr_vring; >> + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ >> + u32 cap_dev_config_size; >> }; >> >> struct ifcvf_adapter { >> -- >> 2.31.1
On 7/13/2022 1:44 PM, Michael S. Tsirkin wrote: > On Fri, Jul 08, 2022 at 02:44:08PM +0800, Zhu, Lingshan wrote: >> >> On 7/4/2022 12:39 PM, Jason Wang wrote: >>> On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: >>>> ifcvf_get_config_size() should return a virtio device type specific value, >>>> however the ret_value should not be greater than the onboard size of >>>> the device implementation. E.g., for virtio_net, config_size should be >>>> the minimum value of sizeof(struct virtio_net_config) and the onboard >>>> cap size. >>> Rethink of this, I wonder what's the value of exposing device >>> implementation details to users? Anyhow the parent is in charge of >>> "emulating" config space accessing. >> This will not be exposed to the users, it is a ifcvf internal helper, >> to get the actual device config space size. >> >> For example, if ifcvf drives an Intel virtio-net device, >> if the device config space size is greater than sizeof(struct >> virtio_net_cfg), >> this means the device has something more than the spec, some private fields, >> we don't want to expose these extra private fields to the users, so in this >> case, >> we only return what the spec defines. > This is kind of already the case. > >> If the device config space size is less than sizeof(struct virtio_net_cfg), >> means the device didn't implement all fields the spec defined, like no RSS. >> In such cases, we only return what the device implemented. >> So these are defensive programming. > I think the issue you are describing is simply this. > > > Driver must not access BAR outside capability length. Current code > does not verify that it does not. Not the case for the current > devices but it's best to be safe against the case where > device does not implement some of the capability. > > > From that POV I think the patch is good, just fix the log. sure, I will do Thanks, Zhu Lingshan > > > >>> If we do this, it's probably a blocker for cross vendor stuff. >>> >>> Thanks >>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- >>>> drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ >>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> index 48c4dadb0c7c..fb957b57941e 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) >>>> break; >>>> case VIRTIO_PCI_CAP_DEVICE_CFG: >>>> hw->dev_cfg = get_cap_addr(hw, &cap); >>>> + hw->cap_dev_config_size = le32_to_cpu(cap.length); >>>> IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); >>>> break; >>>> } >>>> @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) >>>> u32 ifcvf_get_config_size(struct ifcvf_hw *hw) >>>> { >>>> struct ifcvf_adapter *adapter; >>>> + u32 net_config_size = sizeof(struct virtio_net_config); >>>> + u32 blk_config_size = sizeof(struct virtio_blk_config); >>>> + u32 cap_size = hw->cap_dev_config_size; >>>> u32 config_size; >>>> >>>> adapter = vf_to_adapter(hw); >>>> + /* If the onboard device config space size is greater than >>>> + * the size of struct virtio_net/blk_config, only the spec >>>> + * implementing contents size is returned, this is very >>>> + * unlikely, defensive programming. >>>> + */ >>>> switch (hw->dev_type) { >>>> case VIRTIO_ID_NET: >>>> - config_size = sizeof(struct virtio_net_config); >>>> + config_size = cap_size >= net_config_size ? net_config_size : cap_size; >>>> break; >>>> case VIRTIO_ID_BLOCK: >>>> - config_size = sizeof(struct virtio_blk_config); >>>> + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; >>>> break; >>>> default: >>>> config_size = 0; >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> index 115b61f4924b..f5563f665cc6 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> @@ -87,6 +87,8 @@ struct ifcvf_hw { >>>> int config_irq; >>>> int vqs_reused_irq; >>>> u16 nr_vring; >>>> + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ >>>> + u32 cap_dev_config_size; >>>> }; >>>> >>>> struct ifcvf_adapter { >>>> -- >>>> 2.31.1 >>>>
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c index 48c4dadb0c7c..fb957b57941e 100644 --- a/drivers/vdpa/ifcvf/ifcvf_base.c +++ b/drivers/vdpa/ifcvf/ifcvf_base.c @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev) break; case VIRTIO_PCI_CAP_DEVICE_CFG: hw->dev_cfg = get_cap_addr(hw, &cap); + hw->cap_dev_config_size = le32_to_cpu(cap.length); IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg); break; } @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features) u32 ifcvf_get_config_size(struct ifcvf_hw *hw) { struct ifcvf_adapter *adapter; + u32 net_config_size = sizeof(struct virtio_net_config); + u32 blk_config_size = sizeof(struct virtio_blk_config); + u32 cap_size = hw->cap_dev_config_size; u32 config_size; adapter = vf_to_adapter(hw); + /* If the onboard device config space size is greater than + * the size of struct virtio_net/blk_config, only the spec + * implementing contents size is returned, this is very + * unlikely, defensive programming. + */ switch (hw->dev_type) { case VIRTIO_ID_NET: - config_size = sizeof(struct virtio_net_config); + config_size = cap_size >= net_config_size ? net_config_size : cap_size; break; case VIRTIO_ID_BLOCK: - config_size = sizeof(struct virtio_blk_config); + config_size = cap_size >= blk_config_size ? blk_config_size : cap_size; break; default: config_size = 0; diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h index 115b61f4924b..f5563f665cc6 100644 --- a/drivers/vdpa/ifcvf/ifcvf_base.h +++ b/drivers/vdpa/ifcvf/ifcvf_base.h @@ -87,6 +87,8 @@ struct ifcvf_hw { int config_irq; int vqs_reused_irq; u16 nr_vring; + /* VIRTIO_PCI_CAP_DEVICE_CFG size */ + u32 cap_dev_config_size; }; struct ifcvf_adapter {
ifcvf_get_config_size() should return a virtio device type specific value, however the ret_value should not be greater than the onboard size of the device implementation. E.g., for virtio_net, config_size should be the minimum value of sizeof(struct virtio_net_config) and the onboard cap size. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++-- drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-)