Re: [PATCH net] net: psample: Fix the netlink skb length

[Chris Mi <cmi@nvidia.com>]

On 2/4/2021 10:21 AM, Jakub Kicinski wrote:
> On Wed,  3 Feb 2021 11:10:28 +0800 Chris Mi wrote:
>> Currently, the netlink skb length only includes metadata and data
>> length. It doesn't include the psample generic netlink header length.
> But what's the bug? Did you see oversized messages on the socket?
Yes.
>   Did
> one of the nla_put() fail?
Yes.
>
>> Fixes: 6ae0a6286171 ("net: Introduce psample, a new genetlink channel for packet sampling")
>> CC: Yotam Gigi <yotam.gi@gmail.com>
>> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>> Signed-off-by: Chris Mi <cmi@nvidia.com>
>> ---
>>   net/psample/psample.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/psample/psample.c b/net/psample/psample.c
>> index 33e238c965bd..807d75f5a40f 100644
>> --- a/net/psample/psample.c
>> +++ b/net/psample/psample.c
>> @@ -363,6 +363,7 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>>   	struct ip_tunnel_info *tun_info;
>>   #endif
>>   	struct sk_buff *nl_skb;
>> +	int header_len;
>>   	int data_len;
>>   	int meta_len;
>>   	void *data;
>> @@ -381,12 +382,13 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>>   		meta_len += psample_tunnel_meta_len(tun_info);
>>   #endif
>>   
>> +	/* psample generic netlink header size */
>> +	header_len = nlmsg_total_size(GENL_HDRLEN + psample_nl_family.hdrsize);
> GENL_HDRLEN is already included by genlmsg_new() and fam->hdrsize is 0
> / uninitialized for psample_nl_family. What am I missing? Ido?
Thanks for pointing this out. If so, it seems this patch is incorrect.
>
>>   	data_len = min(skb->len, trunc_size);
>> -	if (meta_len + nla_total_size(data_len) > PSAMPLE_MAX_PACKET_SIZE)
>> -		data_len = PSAMPLE_MAX_PACKET_SIZE - meta_len - NLA_HDRLEN
>> +	if (header_len + meta_len + nla_total_size(data_len) > PSAMPLE_MAX_PACKET_SIZE)
>> +		data_len = PSAMPLE_MAX_PACKET_SIZE - header_len - meta_len - NLA_HDRLEN
>>   			    - NLA_ALIGNTO;
>> -
>> -	nl_skb = genlmsg_new(meta_len + nla_total_size(data_len), GFP_ATOMIC);
>> +	nl_skb = genlmsg_new(header_len + meta_len + nla_total_size(data_len), GFP_ATOMIC);
>>   	if (unlikely(!nl_skb))
>>   		return;
>>