On 7/7/21 8:04 AM, Vasily Averin wrote:
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index ff4f9eb..e5af740 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -61,9 +61,24 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
> struct dst_entry *dst = skb_dst(skb);
> struct net_device *dev = dst->dev;
> const struct in6_addr *nexthop;
> + unsigned int hh_len = LL_RESERVED_SPACE(dev);
> struct neighbour *neigh;
> int ret;
>
> + /* Be paranoid, rather than too clever. */
> + if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
> + struct sk_buff *skb2;
> +
> + skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev));
why not use hh_len here?
> + if (!skb2) {
> + kfree_skb(skb);
> + return -ENOMEM;
> + }
> + if (skb->sk)
> + skb_set_owner_w(skb2, skb->sk);
> + consume_skb(skb);
> + skb = skb2;
> + }
> if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
> struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
>
>
On Wed, 7 Jul 2021 08:45:13 -0600 David Ahern wrote:
> On 7/7/21 8:04 AM, Vasily Averin wrote:
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index ff4f9eb..e5af740 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
> > @@ -61,9 +61,24 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
> > struct dst_entry *dst = skb_dst(skb);
> > struct net_device *dev = dst->dev;
> > const struct in6_addr *nexthop;
> > + unsigned int hh_len = LL_RESERVED_SPACE(dev);
> > struct neighbour *neigh;
> > int ret;
> >
> > + /* Be paranoid, rather than too clever. */
> > + if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
> > + struct sk_buff *skb2;
> > +
> > + skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev));
>
> why not use hh_len here?
Is there a reason for the new skb? Why not pskb_expand_head()?
> > + if (!skb2) {
> > + kfree_skb(skb);
> > + return -ENOMEM;
> > + }
> > + if (skb->sk)
> > + skb_set_owner_w(skb2, skb->sk);
> > + consume_skb(skb);
> > + skb = skb2;
> > + }
> > if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
> > struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
On 7/7/21 6:42 PM, Jakub Kicinski wrote:
> On Wed, 7 Jul 2021 08:45:13 -0600 David Ahern wrote:
>> On 7/7/21 8:04 AM, Vasily Averin wrote:
>>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>>> index ff4f9eb..e5af740 100644
>>> --- a/net/ipv6/ip6_output.c
>>> +++ b/net/ipv6/ip6_output.c
>>> @@ -61,9 +61,24 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
>>> struct dst_entry *dst = skb_dst(skb);
>>> struct net_device *dev = dst->dev;
>>> const struct in6_addr *nexthop;
>>> + unsigned int hh_len = LL_RESERVED_SPACE(dev);
>>> struct neighbour *neigh;
>>> int ret;
>>>
>>> + /* Be paranoid, rather than too clever. */
>>> + if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
>>> + struct sk_buff *skb2;
>>> +
>>> + skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev));
>>
>> why not use hh_len here?
>
> Is there a reason for the new skb? Why not pskb_expand_head()?
pskb_expand_head() might crash, if skb is shared.
We possibly can add a helper, factorizing all this,
and eventually use pskb_expand_head() if safe.
Recently Syzkaller found one more issue on RHEL7-based OpenVz kernels.
During its investigation I've found that upstream is affected too.
TEE target send sbk with small headroom into another interface which requires
an increased headroom.
ipv4 handles this problem in ip_finish_output2() and creates new skb with enough headroom,
though ip6_finish_output2() lacks this logic.
Suzkaller created C reproducer, it can be found in v1 cover-letter.
v2 changes:
new helper was created and used in ip6_finish_output2 and in ip6_xmit()
small refactoring in changed functions: commonly used dereferences was replaced by variables
ToDo:
clarify proper name for helper,
move it into proper place,
use it in other similar places:
pptp_xmit
vrf_finish_output
ax25_transmit_buffer
ax25_rt_build_path
bpf_out_neigh_v6
bpf_out_neigh_v4
ip_finish_output2
ip6_tnl_xmit
ipip6_tunnel_xmit
ip_vs_prepare_tunneled_skb
Vasily Averin (4):
ipv6: allocate enough headroom in ip6_finish_output2()
ipv6: use new helper skb_expand_head() in ip6_xmit()
ipv6: ip6_finish_output2 refactoring
ipv6: ip6_xmit refactoring
net/ipv6/ip6_output.c | 89 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 30 deletions(-)
--
1.8.3.1
Recently Syzkaller found one more issue on RHEL7-based OpenVz kernels.
During its investigation I've found that upstream is affected too.
TEE target send sbk with small headroom into another interface which requires
an increased headroom.
ipv4 handles this problem in ip_finish_output2() and creates new skb with enough headroom,
though ip6_finish_output2() lacks this logic.
Suzkaller created C reproducer, it can be found in v1 cover-letter
https://lkml.org/lkml/2021/7/7/467
v3 changes:
now I think it's better to separate bugfix itself and creation of new helper.
now bugfix does not create new inline function. Unlike from v1 it creates new skb
only when it is necessary, i.e. for shared skb only.
In case of failure it updates IPSTATS_MIB_OUTDISCARDS counter
Patch set with new helper will be sent separately.
v2 changes:
new helper was created and used in ip6_finish_output2 and in ip6_xmit()
small refactoring in changed functions: commonly used dereferences was replaced by variables
Vasily Averin (1):
ipv6: allocate enough headroom in ip6_finish_output2()
net/ipv6/ip6_output.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
--
1.8.3.1
currently if skb does not have enough headroom skb_realloc_headrom is called.
It is not optimal because it creates new skb.
Unlike skb_realloc_headroom, new helper pskb_realloc_headroom
does not allocate a new skb if possible;
copies skb->sk on new skb when as needed
and frees original skb in case of failures.
This helps to simplify ip[6]_finish_output2(), ip6_xmit() and a few other
functions in vrf, ax25 and bpf.
There are few other cases where this helper can be used but they require
an additional investigations.
NB: patch "ipv6: use pskb_realloc_headroom in ip6_finish_output2" depends on
patch "ipv6: allocate enough headroom in ip6_finish_output2()" submitted separately
https://lkml.org/lkml/2021/7/12/732
Vasily Averin (7):
skbuff: introduce pskb_realloc_headroom()
ipv6: use pskb_realloc_headroom in ip6_finish_output2
ipv6: use pskb_realloc_headroom in ip6_xmit
ipv4: use pskb_realloc_headroom in ip_finish_output2
vrf: use pskb_realloc_headroom in vrf_finish_output
ax25: use pskb_realloc_headroom
bpf: use pskb_realloc_headroom in bpf_out_neigh_v4/6
drivers/net/vrf.c | 14 +++------
include/linux/skbuff.h | 2 ++
net/ax25/ax25_out.c | 13 +++------
net/ax25/ax25_route.c | 13 +++------
net/core/filter.c | 22 +++-----------
net/core/skbuff.c | 41 ++++++++++++++++++++++++++
net/ipv4/ip_output.c | 12 ++------
net/ipv6/ip6_output.c | 78 ++++++++++++++++++--------------------------------
8 files changed, 89 insertions(+), 106 deletions(-)
--
1.8.3.1