Re: [PATCH 10/25] iwlwifi: mvm: support packet injection

[Kalle Valo]

"Sharon, Sara"  writes:

>> The commit log doesn't tell a lot. I started to wonder why use
>> debugfs and not the proper interface through mac80211?
>
> This is RX packet injection, not TX. So the injection goes up - from
> the driver to mac80211. Since we use it to test some mvm data path
> stuff, we don't want to inject at mac80211 level.
>
>> But reading from the code makes me suspect that this isn't really
>> normal packet injection, more like passing full descriptors to the
>> hw. Did I understand correctly?
>
> For injecting at driver level we need the injected packet to contain a
> fake of the HW descriptor for the driver processing. This descriptor
> isn't being passed to the hw, but to the driver since it is RX. The
> code isn't creating the descriptor, but it is validating length and
> other checks to make sure we won't access invalid memory or so.

Ah, makes sense. No need to resend this, but in the future please try to
explain higher level design like this in the commit log. That avoids a
lot of guessing (wrong) for me and others.

-- 
Kalle Valo

Re: [PATCH 10/25] iwlwifi: mvm: support packet injection

[Kalle Valo]

Luca Coelho  writes:

> From: Sara Sharon 
>
> For automatic testing packet injection can be useful.
> Support injection through debugfs.
>
> Signed-off-by: Sara Sharon 
> Signed-off-by: Luca Coelho 

The commit log doesn't tell a lot. I started to wonder why use debugfs
and not the proper interface through mac80211?

> --- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
> @@ -917,6 +917,59 @@ static ssize_t iwl_dbgfs_indirection_tbl_write(struct 
> iwl_mvm *mvm,
>   return ret ?: count;
>  }
>  
> +static ssize_t iwl_dbgfs_inject_packet_write(struct iwl_mvm *mvm,
> +  char *buf, size_t count,
> +  loff_t *ppos)
> +{
> + struct iwl_rx_cmd_buffer rxb = {
> + ._rx_page_order = 0,
> + .truesize = 0, /* not used */
> + ._offset = 0,
> + };
> + struct iwl_rx_packet *pkt;
> + struct iwl_rx_mpdu_desc *desc;
> + int bin_len = count / 2;
> + int ret = -EINVAL;
> +
> + /* supporting only 9000 descriptor */
> + if (!mvm->trans->cfg->mq_rx_supported)
> + return -ENOTSUPP;
> +
> + rxb._page = alloc_pages(GFP_ATOMIC, 0);
> + if (!rxb._page)
> + return -ENOMEM;
> + pkt = rxb_addr();
> +
> + ret = hex2bin(page_address(rxb._page), buf, bin_len);
> + if (ret)
> + goto out;
> +
> + /* avoid invalid memory access */
> + if (bin_len < sizeof(*pkt) + sizeof(*desc))
> + goto out;
> +
> + /* check this is RX packet */
> + if (WIDE_ID(pkt->hdr.group_id, pkt->hdr.cmd) !=
> + WIDE_ID(LEGACY_GROUP, REPLY_RX_MPDU_CMD))
> + goto out;
> +
> + /* check the length in metadata matches actual received length */
> + desc = (void *)pkt->data;
> + if (le16_to_cpu(desc->mpdu_len) !=
> + (bin_len - sizeof(*desc) - sizeof(*pkt)))
> + goto out;
> +
> + local_bh_disable();
> + iwl_mvm_rx_mpdu_mq(mvm, NULL, , 0);
> + local_bh_enable();
> + ret = 0;

But reading from the code makes me suspect that this isn't really normal
packet injection, more like passing full descriptors to the hw. Did I
understand correctly?

-- 
Kalle Valo