Re: [Intel-wired-lan] [PATCH iwl-next v4 05/12] ice: Log virtual channel messages in PF

[Jacob Keller <jacob.e.keller@intel.com>]

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.liu@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.liu@intel.com>
>> Signed-off-by: Yahui Cao <yahui.cao@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.