On Wed, Mar 05, 2025 at 08:00:22AM -0500, Faizal Rahim wrote:
Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
Co-developed-by: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx>
Signed-off-by: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx>
Co-developed-by: Chwee-Lin Choong <chwee.lin.choong@xxxxxxxxx>
Signed-off-by: Chwee-Lin Choong <chwee.lin.choong@xxxxxxxxx>
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@xxxxxxxxxxxxxxx>
---
+
+static inline bool igc_fpe_is_verify_or_response(union igc_adv_rx_desc *rx_desc,
+ unsigned int size, void *pktbuf)
+{
+ u32 status_error = le32_to_cpu(rx_desc->wb.upper.status_error);
+ static const u8 zero_payload[SMD_FRAME_SIZE] = {0};
+ int smd;
+
+ smd = FIELD_GET(IGC_RXDADV_STAT_SMD_TYPE_MASK, status_error);
+
+ return (smd == IGC_RXD_STAT_SMD_TYPE_V || smd == IGC_RXD_STAT_SMD_TYPE_R) &&
+ size == SMD_FRAME_SIZE &&
+ !memcmp(pktbuf, zero_payload, SMD_FRAME_SIZE); /* Buffer is all zeros */
Using this definition...
+}
+
+static inline void igc_fpe_lp_event_status(union igc_adv_rx_desc *rx_desc,
+ struct ethtool_mmsv *mmsv)
+{
+ u32 status_error = le32_to_cpu(rx_desc->wb.upper.status_error);
+ int smd;
+
+ smd = FIELD_GET(IGC_RXDADV_STAT_SMD_TYPE_MASK, status_error);
+
+ if (smd == IGC_RXD_STAT_SMD_TYPE_V)
+ ethtool_mmsv_event_handle(mmsv, ETHTOOL_MMSV_LP_SENT_VERIFY_MPACKET);
+ else if (smd == IGC_RXD_STAT_SMD_TYPE_R)
+ ethtool_mmsv_event_handle(mmsv, ETHTOOL_MMSV_LP_SENT_RESPONSE_MPACKET);
+}
@@ -2617,6 +2617,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
size -= IGC_TS_HDR_LEN;
}
+ if (igc_fpe_is_pmac_enabled(adapter) &&
+ igc_fpe_is_verify_or_response(rx_desc, size, pktbuf)) {
... invalid SMD-R and SMD-V frames will skip this code block altogether, and
will be passed up the network stack, and visible at least in tcpdump, correct?
Essentially, if the link partner would craft an ICMP request packet with
an SMD-V or SMD-R, your station would respond to it, which is incorrect.
A bit strange, the behavior in this case seems a bit under-specified in
the standard, and I don't see any counter that should be incremented.
+ igc_fpe_lp_event_status(rx_desc, &adapter->fpe.mmsv);
+ /* Advance the ring next-to-clean */
+ igc_is_non_eop(rx_ring, rx_desc);
+ cleaned_count++;
+ continue;
+ }
To fix this, don't you want to merge the unnaturally split
igc_fpe_is_verify_or_response() and igc_fpe_lp_event_status() into a
single function, which returns true whenever the mPacket should be
consumed by the driver, but decides whether to emit a mmsv event on its
own? Merging the two would also avoid reading rx_desc->wb.upper.status_error
twice.
Something like this:
static inline bool igc_fpe_handle_mpacket(struct igc_adapter *adapter,
union igc_adv_rx_desc *rx_desc,
unsigned int size, void *pktbuf)
{
u32 status_error = le32_to_cpu(rx_desc->wb.upper.status_error);
int smd;
smd = FIELD_GET(IGC_RXDADV_STAT_SMD_TYPE_MASK, status_error);
if (smd != IGC_RXD_STAT_SMD_TYPE_V && smd != IGC_RXD_STAT_SMD_TYPE_R)
return false;
if (size == SMD_FRAME_SIZE && mem_is_zero(pktbuf, SMD_FRAME_SIZE)) {
struct ethtool_mmsv *mmsv = &adapter->fpe.mmsv;
enum ethtool_mmsv_event event;
if (smd == IGC_RXD_STAT_SMD_TYPE_V)
event = ETHTOOL_MMSV_LP_SENT_VERIFY_MPACKET;
else
event = ETHTOOL_MMSV_LP_SENT_RESPONSE_MPACKET;
ethtool_mmsv_event_handle(mmsv, event);
}
return true;
}
if (igc_fpe_is_pmac_enabled(adapter) &&
igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf)) {
/* Advance the ring next-to-clean */
igc_is_non_eop(rx_ring, rx_desc);
cleaned_count++;
continue;
}
[ also remark the use of mem_is_zero() instead of memcmp() with a buffer
pre-filled with zeroes. It should be more efficient, for the simple
reason that it's accessing a single memory buffer and not two. Though
I'm surprised how widespread the memcmp() pattern is throughout the
kernel. ]
