* [PATCH net] net: psample: Fix the netlink skb length
@ 2021-02-03 3:10 Chris Mi
2021-02-04 2:21 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Chris Mi @ 2021-02-03 3:10 UTC (permalink / raw)
To: netdev; +Cc: idosch, Chris Mi, Yotam Gigi
Currently, the netlink skb length only includes metadata and data
length. It doesn't include the psample generic netlink header length.
Fix it by adding it.
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);
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;
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net: psample: Fix the netlink skb length 2021-02-03 3:10 [PATCH net] net: psample: Fix the netlink skb length Chris Mi @ 2021-02-04 2:21 ` Jakub Kicinski 2021-02-04 8:47 ` Ido Schimmel 2021-02-04 9:23 ` Chris Mi 0 siblings, 2 replies; 5+ messages in thread From: Jakub Kicinski @ 2021-02-04 2:21 UTC (permalink / raw) To: Chris Mi; +Cc: netdev, idosch, Yotam Gigi 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? Did one of the nla_put() fail? > 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? > 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; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: psample: Fix the netlink skb length 2021-02-04 2:21 ` Jakub Kicinski @ 2021-02-04 8:47 ` Ido Schimmel 2021-02-04 9:32 ` Chris Mi 2021-02-04 9:23 ` Chris Mi 1 sibling, 1 reply; 5+ messages in thread From: Ido Schimmel @ 2021-02-04 8:47 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Chris Mi, netdev, idosch, Yotam Gigi On Wed, Feb 03, 2021 at 06:21:03PM -0800, 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? Did > one of the nla_put() fail? I didn't ask, but I assumed the problem was nla_put(). Agree it needs to be noted in the commit message. > > > 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? Yea, I missed that genlmsg_new() eventually accounts for 'GENL_HDRLEN'. Chris, assuming the problem is nla_put(), I think some other attribute is not accounted for when calculating the size of the skb. Does it only happen with packets that include tunnel metadata? Because I think I see a few problems there: diff --git a/net/psample/psample.c b/net/psample/psample.c index 33e238c965bd..1a233cd128c7 100644 --- a/net/psample/psample.c +++ b/net/psample/psample.c @@ -311,8 +311,10 @@ static int psample_tunnel_meta_len(struct ip_tunnel_info *tun_info) int tun_opts_len = tun_info->options_len; int sum = 0; + sum += nla_total_size(0); /* PSAMPLE_ATTR_TUNNEL */ + if (tun_key->tun_flags & TUNNEL_KEY) - sum += nla_total_size(sizeof(u64)); + sum += nla_total_size_64bit(sizeof(u64)); if (tun_info->mode & IP_TUNNEL_INFO_BRIDGE) sum += nla_total_size(0); > > > 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; > > > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: psample: Fix the netlink skb length 2021-02-04 8:47 ` Ido Schimmel @ 2021-02-04 9:32 ` Chris Mi 0 siblings, 0 replies; 5+ messages in thread From: Chris Mi @ 2021-02-04 9:32 UTC (permalink / raw) To: Ido Schimmel, Jakub Kicinski; +Cc: netdev, idosch, Yotam Gigi On 2/4/2021 4:47 PM, Ido Schimmel wrote: > On Wed, Feb 03, 2021 at 06:21:03PM -0800, 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? Did >> one of the nla_put() fail? > I didn't ask, but I assumed the problem was nla_put(). Agree it needs to > be noted in the commit message. > >>> 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? > Yea, I missed that genlmsg_new() eventually accounts for 'GENL_HDRLEN'. > > Chris, assuming the problem is nla_put(), I think some other attribute > is not accounted for when calculating the size of the skb. Does it only > happen with packets that include tunnel metadata? Yes. > Because I think I see > a few problems there: > > diff --git a/net/psample/psample.c b/net/psample/psample.c > index 33e238c965bd..1a233cd128c7 100644 > --- a/net/psample/psample.c > +++ b/net/psample/psample.c > @@ -311,8 +311,10 @@ static int psample_tunnel_meta_len(struct ip_tunnel_info *tun_info) > int tun_opts_len = tun_info->options_len; > int sum = 0; > > + sum += nla_total_size(0); /* PSAMPLE_ATTR_TUNNEL */ > + > if (tun_key->tun_flags & TUNNEL_KEY) > - sum += nla_total_size(sizeof(u64)); > + sum += nla_total_size_64bit(sizeof(u64)); > > if (tun_info->mode & IP_TUNNEL_INFO_BRIDGE) > sum += nla_total_size(0); Thanks for this patch. I'll check it. BTW, maybe I should not mention it, if we have the psample dependency removal patch which is rejected, I think we can debug the psample issue easily. Because we can unload and load psample easily. But if NIC driver calls psample api directly. We have to unload the driver first. After loading the NIC driver, we have to enable sriov and enable switchdev mode again which is time consuming. >>> 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; >>> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: psample: Fix the netlink skb length 2021-02-04 2:21 ` Jakub Kicinski 2021-02-04 8:47 ` Ido Schimmel @ 2021-02-04 9:23 ` Chris Mi 1 sibling, 0 replies; 5+ messages in thread From: Chris Mi @ 2021-02-04 9:23 UTC (permalink / raw) To: Jakub Kicinski; +Cc: netdev, idosch, Yotam Gigi 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; >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-04 9:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-03 3:10 [PATCH net] net: psample: Fix the netlink skb length Chris Mi 2021-02-04 2:21 ` Jakub Kicinski 2021-02-04 8:47 ` Ido Schimmel 2021-02-04 9:32 ` Chris Mi 2021-02-04 9:23 ` Chris Mi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).