Re: [Intel-wired-lan] [PATCH net] i40e: limit the DDP profile count returned by the firmware

[Linkui Xiao]

On 2026/9/16 14:03, Loktionov, Aleksandr wrote:
> > -----Original Message-----
> > From: Linkui Xiao <xiaolinkui@xxxxxxx>
> > Sent: Tuesday, September 15, 2026 2:04 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel,
> > Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; andrew+netdev@xxxxxxx;
> > davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> > pabeni@xxxxxxxxxx
> > Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> > Subject: [Intel-wired-lan] [PATCH net] i40e: limit the DDP profile
> > count returned by the firmware
> >
> > From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> >
> > i40e_aq_get_ddp_list() writes into a I40E_PROFILE_LIST_SIZE buffer,
> > which is sized for I40E_MAX_PROFILE_NUM (16) i40e_profile_info entries
> > plus the 4 byte p_count header. i40e_ddp_does_profile_exist() and
> > i40e_ddp_does_profile_overlap() then loop over profile_list->p_count
> > without bounding it, so a firmware reporting more than 16 profiles
> > makes both helpers walk past the end of the on-stack buff[] and
> > compare against whatever happens to follow it on the stack.
> >
> > Clamp the count to the number of entries the buffer can actually hold
> > and make the loop counter unsigned to match the field type.
> >
> > Fixes: cdc594e00370 ("i40e: Implement DDP support in i40e driver")
> > Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/intel/i40e/i40e_ddp.c | 18 ++++++++++++++----
> >   1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> > b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> > index daa9f2c42f70..1d6d8b3835a3 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> > @@ -55,7 +55,7 @@ static int i40e_ddp_does_profile_exist(struct
> > i40e_hw *hw,
> >   	struct i40e_ddp_profile_list *profile_list;
> >   	u8 buff[I40E_PROFILE_LIST_SIZE];
> >   	int status;
> > -	int i;
> > +	u32 i, p_count;
> RCT please
>
> >   	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE,
> > 0,
> >   				      NULL);
> > @@ -63,7 +63,12 @@ static int i40e_ddp_does_profile_exist(struct
> > i40e_hw *hw,
> >   		return -1;
> >
> >   	profile_list = (struct i40e_ddp_profile_list *)buff;
> > -	for (i = 0; i < profile_list->p_count; i++) {
> > +	/* Never walk past the end of buff[], the profile count
> > reported by
> > +	 * the firmware is not guaranteed to fit into the buffer we
> > gave it.
> > +	 */
> > +	p_count = min_t(u32, profile_list->p_count,
> > I40E_MAX_PROFILE_NUM);
> > +
> > +	for (i = 0; i < p_count; i++) {
> >   		if (i40e_ddp_profiles_eq(pinfo, &profile_list-
> > > p_info[i]))
> >   			return 1;
> >   	}
> > @@ -110,7 +115,7 @@ static int i40e_ddp_does_profile_overlap(struct
> > i40e_hw *hw,
> >   	struct i40e_ddp_profile_list *profile_list;
> >   	u8 buff[I40E_PROFILE_LIST_SIZE];
> >   	int status;
> > -	int i;
> > +	u32 i, p_count;
> RCT please
>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>
Hi,

Thanks for the review and for the Reviewed-by tag.

Just to make sure I understand the "RCT please" comments correctly: are 
they referring to the Reverse Christmas Tree (RCT) variable declaration 
ordering convention, i.e. the local variables should be declared in 
decreasing order of line length? In both functions I currently have:
        struct i40e_ddp_profile_list *profile_list;
        u8 buff[I40E_PROFILE_LIST_SIZE];
        int status;
        u32 i, p_count;

If I understand RCT correctly, the expected order would be something like:
        struct i40e_ddp_profile_list *profile_list;
        u8 buff[I40E_PROFILE_LIST_SIZE];
        u32 i, p_count;
        int status;

Could you please confirm whether that is what is being asked for, and 
whether there are any other RCT-related issues in this patch?
Also, should I send a v2 with the RCT ordering fixed, or would you 
prefer to keep the current version as-is since the change is purely 
stylistic? I am happy to send a v2 if that is preferred.

Thanks,
Linkui
> >   	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE,
> > 0,
> >   				      NULL);
> > @@ -118,7 +123,12 @@ static int i40e_ddp_does_profile_overlap(struct
> > i40e_hw *hw,
> >   		return -EIO;
> >
> >   	profile_list = (struct i40e_ddp_profile_list *)buff;
> > -	for (i = 0; i < profile_list->p_count; i++) {
> > +	/* Never walk past the end of buff[], the profile count
> > reported by
> > +	 * the firmware is not guaranteed to fit into the buffer we
> > gave it.
> > +	 */
> > +	p_count = min_t(u32, profile_list->p_count,
> > I40E_MAX_PROFILE_NUM);
> > +
> > +	for (i = 0; i < p_count; i++) {
> >   		if (i40e_ddp_profiles_overlap(pinfo,
> >   					      &profile_list->p_info[i]))
> >   			return 1;
> > --
> > 2.25.1