On 12/7/2023 5:53 PM, Brett Creeley wrote:
>
>
> On 11/20/2023 6:51 PM, Yahui Cao wrote:
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> From: Lingyu Liu <lingyu....@intel.com>
>>
>> Save the virtual channel messages sent by VF on the source side during
>> runtime. The logged virtchnl messages will be transferred and loaded
>> into the device on the destination side during the device resume stage.
>>
>> For the feature which can not be migrated yet, it must be disabled or
>> blocked to prevent from being abused by VF. Otherwise, it may introduce
>> functional and security issue. Mask unsupported VF capability flags in
>> the VF-PF negotiaion stage.
>
> s/negotiaion/negotiation/
>
>>
>> Signed-off-by: Lingyu Liu <lingyu....@intel.com>
>> Signed-off-by: Yahui Cao <yahui....@intel.com>
>> ---
>> .../net/ethernet/intel/ice/ice_migration.c | 167 ++++++++++++++++++
>> .../intel/ice/ice_migration_private.h | 17 ++
>> drivers/net/ethernet/intel/ice/ice_vf_lib.h | 5 +
>> drivers/net/ethernet/intel/ice/ice_virtchnl.c | 31 ++++
>> 4 files changed, 220 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_migration.c
>> b/drivers/net/ethernet/intel/ice/ice_migration.c
>> index 2b9b5a2ce367..18ec4ec7d147 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_migration.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_migration.c
>> @@ -3,6 +3,17 @@
>>
>> #include "ice.h"
>>
>> +struct ice_migration_virtchnl_msg_slot {
>> + u32 opcode;
>> + u16 msg_len;
>> + char msg_buffer[];
>> +};
>> +
>> +struct ice_migration_virtchnl_msg_listnode {
>> + struct list_head node;
>> + struct ice_migration_virtchnl_msg_slot msg_slot;
>> +};
>> +
>> /**
>> * ice_migration_get_pf - Get ice PF structure pointer by pdev
>> * @pdev: pointer to ice vfio pci VF pdev structure
>> @@ -22,6 +33,9 @@ EXPORT_SYMBOL(ice_migration_get_pf);
>> void ice_migration_init_vf(struct ice_vf *vf)
>> {
>> vf->migration_enabled = true;
>> + INIT_LIST_HEAD(&vf->virtchnl_msg_list);
>> + vf->virtchnl_msg_num = 0;
>> + vf->virtchnl_msg_size = 0;
>> }
>>
>> /**
>> @@ -30,10 +44,24 @@ void ice_migration_init_vf(struct ice_vf *vf)
>> */
>> void ice_migration_uninit_vf(struct ice_vf *vf)
>> {
>> + struct ice_migration_virtchnl_msg_listnode *msg_listnode;
>> + struct ice_migration_virtchnl_msg_listnode *dtmp;
>> +
>> if (!vf->migration_enabled)
>> return;
>>
>> vf->migration_enabled = false;
>> +
>> + if (list_empty(&vf->virtchnl_msg_list))
>> + return;
>> + list_for_each_entry_safe(msg_listnode, dtmp,
>> + &vf->virtchnl_msg_list,
>> + node) {
>> + list_del(&msg_listnode->node);
>> + kfree(msg_listnode);
>> + }
>> + vf->virtchnl_msg_num = 0;
>> + vf->virtchnl_msg_size = 0;
>> }
>>
>> /**
>> @@ -80,3 +108,142 @@ void ice_migration_uninit_dev(struct ice_pf *pf, int
>> vf_id)
>> ice_put_vf(vf);
>> }
>> EXPORT_SYMBOL(ice_migration_uninit_dev);
>> +
>> +/**
>> + * ice_migration_is_loggable_msg - is this message loggable or not
>> + * @v_opcode: virtchnl message operation code
>> + *
>> + * Return true if this message logging is supported, otherwise return false
>> + */
>> +static inline bool ice_migration_is_loggable_msg(u32 v_opcode)
>> +{
>> + switch (v_opcode) {
>> + case VIRTCHNL_OP_VERSION:
>> + case VIRTCHNL_OP_GET_VF_RESOURCES:
>> + case VIRTCHNL_OP_CONFIG_VSI_QUEUES:
>> + case VIRTCHNL_OP_CONFIG_IRQ_MAP:
>> + case VIRTCHNL_OP_ADD_ETH_ADDR:
>> + case VIRTCHNL_OP_DEL_ETH_ADDR:
>> + case VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE:
>> + case VIRTCHNL_OP_ENABLE_QUEUES:
>> + case VIRTCHNL_OP_DISABLE_QUEUES:
>> + case VIRTCHNL_OP_ADD_VLAN:
>> + case VIRTCHNL_OP_DEL_VLAN:
>> + case VIRTCHNL_OP_ENABLE_VLAN_STRIPPING:
>> + case VIRTCHNL_OP_DISABLE_VLAN_STRIPPING:
>> + case VIRTCHNL_OP_CONFIG_RSS_KEY:
>> + case VIRTCHNL_OP_CONFIG_RSS_LUT:
>> + case VIRTCHNL_OP_GET_SUPPORTED_RXDIDS:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> +/**
>> + * ice_migration_log_vf_msg - Log request message from VF
>> + * @vf: pointer to the VF structure
>> + * @event: pointer to the AQ event
>> + *
>> + * Log VF message for later device state loading during live migration
>> + *
>> + * Return 0 for success, negative for error
>> + */
>> +int ice_migration_log_vf_msg(struct ice_vf *vf,
>> + struct ice_rq_event_info *event)
>> +{
>> + struct ice_migration_virtchnl_msg_listnode *msg_listnode;
>> + u32 v_opcode = le32_to_cpu(event->desc.cookie_high);
>> + struct device *dev = ice_pf_to_dev(vf->pf);
>> + u16 msglen = event->msg_len;
>> + u8 *msg = event->msg_buf;
>> +
>> + if (!ice_migration_is_loggable_msg(v_opcode))
>> + return 0;
>> +
>> + if (vf->virtchnl_msg_num >= VIRTCHNL_MSG_MAX) {
>> + dev_warn(dev, "VF %d has maximum number virtual channel
>> commands\n",
>> + vf->vf_id);
>> + return -ENOMEM;
>> + }
>> +
>> + msg_listnode = (struct ice_migration_virtchnl_msg_listnode *)
>> + kzalloc(struct_size(msg_listnode,
>> + msg_slot.msg_buffer,
>> + msglen),
>> + GFP_KERNEL);
>> + if (!msg_listnode) {
>> + dev_err(dev, "VF %d failed to allocate memory for msg
>> listnode\n",
>> + vf->vf_id);
>> + return -ENOMEM;
>> + }
>> + dev_dbg(dev, "VF %d save virtual channel command, op code: %d, len:
>> %d\n",
>> + vf->vf_id, v_opcode, msglen);
>> + msg_listnode->msg_slot.opcode = v_opcode;
>> + msg_listnode->msg_slot.msg_len = msglen;
>> + memcpy(msg_listnode->msg_slot.msg_buffer, msg, msglen);
>
> It seems like this can still be abused. What if the VM/VF user sends
> hundreds of thousands of ADD_ADDR/DEL_ADDR, ADD_VLAN/DEL_VLAN,
> PROMISCUOUS, ENABLE_VLAN_STRIPPING/DISABLE_VLAN_STRIPPING, RSS_LUT,
> RSS_KEY, etc.?
>
> Shouldn't you only maintain one copy for each key/value when it makes
> sense? For example, you don't need multiple RSS_LUT and RSS_KEY messages
> logged as just the most recent one is needed.
>
> What if multiple promiscuous messages are sent? Do you need to save them
> all or just the most recent?
>
> What if you have an ADD_ADDR/DEL_ADDR for the same address? Do you need
> to save both of those messages? Seems like when you get a DEL_ADDR you
> should search for the associated ADD_ADDR and just remove it. Same
> comment applies for ADD_VLAN/DEL_VLAN.
>
I'll likely be looking to refactor this to use a new internal
representation of the VF state instead of a list of virtchnl messages.
