Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v4 1/3] VSOCK: Add vsockmon tap functions
From: Jorgen S. Hansen @ 2017-04-20 12:27 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: netdev@vger.kernel.org, Zhu Yanjun, Michael S. Tsirkin,
	Gerard Garcia
In-Reply-To: <20170413161811.8953-2-stefanha@redhat.com>


> On Apr 13, 2017, at 6:18 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> +
> +static void __vsock_deliver_tap(struct sk_buff *skb)
> +{
> +	int ret;
> +	struct vsock_tap *tmp;
> +
> +	list_for_each_entry_rcu(tmp, &vsock_tap_all, list) {
> +		ret = __vsock_deliver_tap_skb(skb, tmp->dev);
> +		if (unlikely(ret))
> +			break;
> +	}
> +
> +	consume_skb(skb);

It looks like the caller will allocate the skb regardless of whether vsock_tap_all is empty, so shouldn’t the skb be consumed in vsock_deliver_tap?

> +}
> +
> +void vsock_deliver_tap(struct sk_buff *skb)
> +{
> +	rcu_read_lock();
> +
> +	if (unlikely(!list_empty(&vsock_tap_all)))
> +		__vsock_deliver_tap(skb);
> +
> +	rcu_read_unlock();
> +}
> +EXPORT_SYMBOL_GPL(vsock_deliver_tap);
> -- 
> 2.9.3
> 


^ permalink raw reply

* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Eric Dumazet @ 2017-04-20 12:18 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: Jiri Pirko, davem, netdev, xiyou.wangcong
In-Reply-To: <f0e84f44-ba3a-7eb4-8b22-9764048d9406@mojatatu.com>

On Thu, 2017-04-20 at 06:42 -0400, Jamal Hadi Salim wrote:

> 
> They are not the same issue Jiri. We have used bitmasks fine on netlink
> message for a millenia. Nobody sets garbage on a bitmask they are not
> supposed to touch. The struct padding thing is a shame the way it
> turned out - now netlink can no longer have a claim to be a (good)
> wire protocol.

Except that users wrote programs, and these programs work today.

By changing the kernel and recognizing new flags in existing padding,
you might break the programs.

This is not acceptable. Period.

Had we checked the padding being 0 in old kernels, this change would
have been possible today.

But because old kernels did not care of the padding contents, then there
is no way new kernel can suddenly trust them at all.

Please Jamal, you have to forget this nonsense.

^ permalink raw reply

* Re: net: heap out-of-bounds in fib6_clean_node/rt6_fill_node/fib6_age/fib6_prune_clone
From: Andrey Konovalov @ 2017-04-20 12:10 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: David Ahern, Cong Wang, Eric Dumazet, Mahesh Bandewar,
	Eric Dumazet, David Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, syzkaller
In-Reply-To: <CACT4Y+amFHj1DV3pmj+eJVxg52+GLdE-fEin5M86dOKubea7DQ@mail.gmail.com>

On Thu, Apr 20, 2017 at 10:35 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Apr 20, 2017 at 1:51 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> On 4/19/17 5:47 PM, Cong Wang wrote:
>>> On Wed, Apr 19, 2017 at 9:12 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>>>>
>>>> Anyway, I just finished simplifying the reproducer. Give this one a try.
>>>
>>> Thanks for providing such a minimal reproducer!
>>>
>>> The following patch could fix this crash, but I am not 100% sure if we should
>>> just clear these bits or reject them with an errno.
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 9db14189..cf524c2 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -2086,7 +2086,7 @@ static struct rt6_info
>>> *ip6_route_info_create(struct fib6_config *cfg)
>>>         } else
>>>                 rt->rt6i_prefsrc.plen = 0;
>>>
>>> -       rt->rt6i_flags = cfg->fc_flags;
>>> +       rt->rt6i_flags = cfg->fc_flags & ~(RTF_PCPU | RTF_CACHE);
>>>
>>>  install_route:
>>>         rt->dst.dev = dev;
>>>
>>
>> I sent a patch returning EINVAL if RTF_PCPU is set in fc_flags
>
>
> Andrey, does it fix the other crashes?

No, still see them.

I'm working on reproducing those.

^ permalink raw reply

* Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable
From: James Hughes @ 2017-04-20 12:09 UTC (permalink / raw)
  To: Kalle Valo
  Cc: netdev, Arend van Spriel, Franky Lin, Hante Meuleman,
	linux-wireless
In-Reply-To: <87y3uv48nv.fsf@kamboji.qca.qualcomm.com>

On 20 April 2017 at 12:31, Kalle Valo <kvalo@codeaurora.org> wrote:
> + linux-wireless
>
> James Hughes <james.hughes@raspberrypi.org> writes:
>
>> The driver was adding header information to incoming skb
>> without ensuring the head was uncloned and hence writable.
>>
>> skb_cow_head has been used to ensure they are writable, however,
>> this required some changes to error handling to ensure that
>> if skb_cow_head failed it was not ignored.
>>
>> This really needs to be reviewed by someone who is more familiar
>> with this code base to ensure any deallocation of skb's is
>> still correct.
>>
>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>
> You should also CC linux-wireless, otherwise patchwork won't see it.
>
> --
> Kalle Valo

Thanks Kalle, I wasn't subscribed to wireless, but have now done so. I
also failed to read the MAINTAINERS list correctly..

With regard to this particular patch, this is related to the recent
patches to use skb_cow_head in a number of USB net drivers to ensure
they can write headers correctly, and I suspect the same fault is
possible/likely in other drivers outside the USB net realm, as this
patch shows.

I'm not overly happy with the error handling in this patch, but that
said, the error handling over this entire driver does strike me as
suspect. Quite a few places where return codes are ignored, just in my
quick examination. So not really sure how to proceed past this patch,
if at all.

^ permalink raw reply

* Re: [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: Andrey Konovalov @ 2017-04-20 12:09 UTC (permalink / raw)
  To: Martin KaFai Lau; +Cc: David Ahern, netdev
In-Reply-To: <20170419215232.2lu7kaebwg75ikn3@giedrius-mbp.dhcp.thefacebook.com>

Thanks!

Tested-by: Andrey Konovalov <andreyknvl@google.com>

On Wed, Apr 19, 2017 at 11:52 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Wed, Apr 19, 2017 at 02:19:43PM -0700, David Ahern wrote:
>> Andrey reported a fault in the IPv6 route code:
>>
>> kasan: GPF could be caused by NULL-ptr deref or user memory access
>> general protection fault: 0000 [#1] SMP KASAN
>> Modules linked in:
>> CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff880069809600 task.stack: ffff880062dc8000
>> RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
>> RSP: 0018:ffff880062dced30 EFLAGS: 00010206
>> RAX: dffffc0000000000 RBX: ffff8800670561c0 RCX: 0000000000000006
>> RDX: 0000000000000003 RSI: ffff880062dcfb28 RDI: 0000000000000018
>> RBP: ffff880062dced68 R08: 0000000000000001 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>> R13: ffff880062dcfb28 R14: dffffc0000000000 R15: 0000000000000000
>> FS:  00007feebe37e7c0(0000) GS:ffff88006cb00000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00000000205a0fe4 CR3: 000000006b5c9000 CR4: 00000000000006e0
>> Call Trace:
>>  ip6_pol_route+0x1512/0x1f20 net/ipv6/route.c:1128
>>  ip6_pol_route_output+0x4c/0x60 net/ipv6/route.c:1212
>> ...
>>
>> Andrey's syzkaller program passes rtmsg.rtmsg_flags with the RTF_PCPU bit
>> set. Flags passed to the kernel are blindly copied to the allocated
>> rt6_info by ip6_route_info_create making a newly inserted route appear
>> as though it is a per-cpu route. ip6_rt_cache_alloc sees the flag set
>> and expects rt->dst.from to be set - which it is not since it is not
>> really a per-cpu copy. The subsequent call to __ip6_dst_alloc then
>> generates the fault.
>>
>> Fix by checking for the flag and failing with EINVAL.
> Thanks for the fix.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
>>
>> Fixes: d52d3997f843f ("ipv6: Create percpu rt6_info")
>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>> ---
>>  include/uapi/linux/ipv6_route.h | 2 +-
>>  net/ipv6/route.c                | 4 ++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/ipv6_route.h b/include/uapi/linux/ipv6_route.h
>> index 85bbb1799df3..d496c02e14bc 100644
>> --- a/include/uapi/linux/ipv6_route.h
>> +++ b/include/uapi/linux/ipv6_route.h
>> @@ -35,7 +35,7 @@
>>  #define RTF_PREF(pref)       ((pref) << 27)
>>  #define RTF_PREF_MASK        0x18000000
>>
>> -#define RTF_PCPU     0x40000000
>> +#define RTF_PCPU     0x40000000      /* read-only: can not be set by user */
>>  #define RTF_LOCAL    0x80000000
>>
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 4ba7c49872ff..a1bf426c959b 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -1854,6 +1854,10 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg)
>>       int addr_type;
>>       int err = -EINVAL;
>>
>> +     /* RTF_PCPU is an internal flag; can not be set by userspace */
>> +     if (cfg->fc_flags & RTF_PCPU)
>> +             goto out;
>> +
>>       if (cfg->fc_dst_len > 128 || cfg->fc_src_len > 128)
>>               goto out;
>>  #ifndef CONFIG_IPV6_SUBTREES
>> --
>> 2.9.3
>>

^ permalink raw reply

* [PATCH v3 net] net sched actions: allocate act cookie early
From: Wolfgang Bumiller @ 2017-04-20 12:08 UTC (permalink / raw)
  To: netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang

Policing filters do not use the TCA_ACT_* enum and the tb[]
nlattr array in tcf_action_init_1() doesn't get filled for
them so we should not try to look for a TCA_ACT_COOKIE
attribute in the then uninitialized array.
The error handling in cookie allocation then calls
tcf_hash_release() leading to invalid memory access later
on.
Additionally, if cookie allocation fails after an already
existing non-policing filter has successfully been changed,
tcf_action_release() should not be called, also we would
have to roll back the changes in the error handling, so
instead we now allocate the cookie early and assign it on
success at the end.

CVE-2017-7979
Fixes: 1045ba77a596 ("net sched actions: Add support for user cookies")
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
This replaces both patches of the previous series:
  - 1/2 net sched actions: fix access to uninitialized data
   Changed: The affected code is now moved up into the first
            (name==NULL) branch rather than adding this condition anew.
  - 2/2 net sched actions: decrement module refcount earlier
   Changed: Instead of moving the module_put above the cookie code,
            the cookie code is moved as described in the commit
            message.

 net/sched/act_api.c | 55 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b70aa57319ea..e05b924618a0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -529,20 +529,20 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
 	return err;
 }
 
-static int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
+static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 {
-	a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
-	if (!a->act_cookie)
-		return -ENOMEM;
+	struct tc_cookie *c = kzalloc(sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return NULL;
 
-	a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
-	if (!a->act_cookie->data) {
-		kfree(a->act_cookie);
-		return -ENOMEM;
+	c->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
+	if (!c->data) {
+		kfree(c);
+		return NULL;
 	}
-	a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
+	c->len = nla_len(tb[TCA_ACT_COOKIE]);
 
-	return 0;
+	return c;
 }
 
 struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
@@ -551,6 +551,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
+	struct tc_cookie *cookie = NULL;
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
@@ -566,6 +567,18 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 			goto err_out;
 		if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
 			goto err_out;
+		if (tb[TCA_ACT_COOKIE]) {
+			int cklen = nla_len(tb[TCA_ACT_COOKIE]);
+
+			if (cklen > TC_COOKIE_MAX_SIZE)
+				goto err_out;
+
+			cookie = nla_memdup_cookie(tb);
+			if (!cookie) {
+				err = -ENOMEM;
+				goto err_out;
+			}
+		}
 	} else {
 		err = -EINVAL;
 		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
@@ -604,20 +617,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
-	if (tb[TCA_ACT_COOKIE]) {
-		int cklen = nla_len(tb[TCA_ACT_COOKIE]);
-
-		if (cklen > TC_COOKIE_MAX_SIZE) {
-			err = -EINVAL;
-			tcf_hash_release(a, bind);
-			goto err_mod;
-		}
-
-		if (nla_memdup_cookie(a, tb) < 0) {
-			err = -ENOMEM;
-			tcf_hash_release(a, bind);
-			goto err_mod;
+	if (name == NULL && tb[TCA_ACT_COOKIE]) {
+		if (a->act_cookie) {
+			kfree(a->act_cookie->data);
+			kfree(a->act_cookie);
 		}
+		a->act_cookie = cookie;
 	}
 
 	/* module count goes up only when brand new policy is created
@@ -632,6 +637,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 err_mod:
 	module_put(a_o->owner);
 err_out:
+	if (cookie) {
+		kfree(cookie->data);
+		kfree(cookie);
+	}
 	return ERR_PTR(err);
 }
 
-- 
2.11.0

^ permalink raw reply related

* Re: [net-next 04/14] i40e: dump VF information in debugfs
From: Jiri Pirko @ 2017-04-20 11:39 UTC (permalink / raw)
  To: Mintz, Yuval
  Cc: Or Gerlitz, Jeff Kirsher, Mitch Williams, David Miller,
	Linux Netdev List, nhorman@redhat.com, sassmann@redhat.com,
	jogreene@redhat.com
In-Reply-To: <BL2PR07MB23064C036E994DC63475F7388D1B0@BL2PR07MB2306.namprd07.prod.outlook.com>

Thu, Apr 20, 2017 at 12:09:28PM CEST, Yuval.Mintz@cavium.com wrote:
>> > Dump some internal state about VFs through debugfs. This provides
>> > information not available with 'ip link show'.
>> 
>> such as?
>> 
>> donnwantobethedebugfspolice, but still, in the 2-3 times we tried to push
>> debugfs to MLNX NIC drivers, Dave disallowed that, and lately the switch
>> team even went further and deleted that portion of the mlxsw driver -- all to
>> all,  I don't see much point for these type of changes, thoughts?
>
>Don't want to hikjack your thread, but continuing this topic -
>Is there some flat-out disapproval for debugfs in net-next now?

It was mentioned by DaveM on the list multiple times.


>
>We're currently internally engaged with adding qed support for register dumps
>[~equivalents for `ethtool -d' outputs] through debugfs, on behalf of storage
>drivers [qedi/qedf] lacking the API for doing that.

That sounds wrong. Either introduce a generic infra to expose the info you
need or you don't do that. For your inhouse debugging, you should have
oot patch to expose whatever you need.

^ permalink raw reply

* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-20 11:35 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <f0e84f44-ba3a-7eb4-8b22-9764048d9406@mojatatu.com>

Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote:
>On 17-04-19 12:17 PM, Jiri Pirko wrote:
>> Wed, Apr 19, 2017 at 06:07:48PM CEST, jhs@mojatatu.com wrote:
>> > On 17-04-19 11:54 AM, Jiri Pirko wrote:
>> > > Wed, Apr 19, 2017 at 05:37:15PM CEST, jhs@mojatatu.com wrote:
>> > > > On 17-04-19 09:13 AM, Jiri Pirko wrote:
>> > > > > Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@mojatatu.com wrote:
>> > > > > > On 17-04-19 08:36 AM, Jiri Pirko wrote:
>> > > > > > > Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>> > > > > > > > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> > > > > > 
>> > > > > > > > include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>> > > > > > > > net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
>> > > > > > > > 3 files changed, 53 insertions(+), 12 deletions(-)
>> > > > 
>> > > > > > > > +#define TCAA_MAX (__TCAA_MAX - 1)
>> > > > > > > > #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> > > > > > > > #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> > > > > > > > -#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>> > > > > > > > -#define TCAA_MAX 1
>> > > > > > > > +#define TCA_ACT_TAB TCAA_ACT_TAB
>> > > > > > > 
>> > > > > > > This is mess. What does "TCAA" stand for?
>> > > > > > 
>> > > > > > TC Actions Attributes.  What would you call it? I could have
>> > > > > > called it TCA_ROOT etc. But maybe a comment to just call it
>> > > > > > TC Actions Attributes would be enough?
>> > > > > 
>> > > > > TCA_DUMP_X
>> > > > > 
>> > > > > it is only for dumping. Naming it "attribute" seems weird. Same as if
>> > > > > you have: int variable_something;
>> > > > > 
>> > > > 
>> > > > Jiri, this is not just for dumping. We are describing high level
>> > > > attributes for tc actions.
>> > > 
>> > > This is already present:
>> > > enum {
>> > >         TCA_ACT_UNSPEC,
>> > >         TCA_ACT_KIND,
>> > >         TCA_ACT_OPTIONS,
>> > >         TCA_ACT_INDEX,
>> > >         TCA_ACT_STATS,
>> > >         TCA_ACT_PAD,
>> > >         TCA_ACT_COOKIE,
>> > >         __TCA_ACT_MAX
>> > > };
>> > > 
>> > > This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB)
>> > > 
>> > > Looks like you are mixing these 2.
>> > > 
>> > 
>> > No - That space is per action. The space i am defining is
>> > above that in the the hierarchy. There used to be only
>> > one attribute there (TCA_ACT_TAB) and now we are making
>> > it more generic.
>> 
>> Right. That's what I say. And that higher level is used only for
>> dumping. That's why I suggested TCA_ACT_DUMP prefix.
>> 
>
>DUMP is not right. _TAB is used for SET/DEL as well.
>why dont we leave this alone so we can progress?
>You can submit changes later if it bothers you still.

Ha. So the current code is wrong, I see it now. Following is needed:

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48..c432b22 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
 			  extack);
 	if (ret < 0)
 		return ret;

You confused me squashing the change to this patch.

Ok, so the name could be:
TCA_ACTS_*
?
I believe it is crucial to figure this out right now. TC UAPI is in deep
mess already, no need to push it even more.


>
>> 
>> > 
>> > > 
>> > > 
>> > 
>> > > > It is a _lot_ of code to change! Note:
>> > > > This is all the UAPI visible code (the same coding style for 20 years).
>> > > > I am worried about this part.
>> > > 
>> > > We'll see. Lets do it in a sensitive way, in steps. But for new things,
>> > > I think it is good not to stick with old and outlived habits.
>> > > 
>> > > 
>> > 
>> > I know you have the muscle to get it done - so fine, i will start
>> > with this one change.
>> > 
>> > > 
>> > > Netlink is TLV, should be used as TLV. I don't see how you can run out
>> > > any space. You tend to use Netlink in some weird hybrid mode, with only
>> > > argument being space. I think that couple of bytes wasted is not
>> > > a problem at all...
>> > > 
>> > 
>> > You are not making sense to me still.
>> > What you describe as "a couple of bytes" adds up when you have
>> > a large number of objects. I am trying to cut down on data back
>> > and forth from user/kernel and a bitmap is a well understood entity.
>> 
>> The attributes in question are per-message, not per-object
>> 
>> 
>> > 
>> > Even if i did use a TLV - when i am representing flags which require one
>> > bit each - it makes sense to use a bitmask encapsulated in a TLV. Not
>> > to waste a TLV per bit.
>> 
>> That is the only correct way. For example, in future the kernel may
>> report in extended ack that it does not support some specific attribute
>> user passed. If you pack it all in one attr, that would not be possible.
>> Also, what prevents user from passing random data on bit flag positions
>> that you are not using? Current kernel would ignore it. Next kernel will
>> do something different according to the flag bit. That is UAPI break.
>> Essentialy the same thing what DaveM said about the struct. You have to
>> define this completely, at the beginning, not possible to use the unused
>> bits for other things in the future.
>> 
>
>
>They are not the same issue Jiri. We have used bitmasks fine on netlink

Howcome they are not the same? I'm pretty certain they are.


>message for a millenia. Nobody sets garbage on a bitmask they are not
>supposed to touch. The struct padding thing is a shame the way it
>turned out - now netlink can no longer have a claim to be a (good)
>wire protocol.
>Other thing: I know you feel strongly about this but I dont agree that
>everything has to be a TLV and in no way, as a matter of principle, you are
>going to convince me  (even when the cows come home) that I have to
>use 64 bits to carry a single bit just so I can use TLVs.

Then I guess we have to agree to disagree. I believe that your approach
is wrong and will break users in future when you add even a single flag.
Argument that "we are doing this for ages-therefore it is correct" has
simply 0 value.

^ permalink raw reply related

* Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable
From: Kalle Valo @ 2017-04-20 11:31 UTC (permalink / raw)
  To: James Hughes
  Cc: netdev, Arend van Spriel, Franky Lin, Hante Meuleman,
	linux-wireless
In-Reply-To: <20170420111651.10213-1-james.hughes@raspberrypi.org>

+ linux-wireless

James Hughes <james.hughes@raspberrypi.org> writes:

> The driver was adding header information to incoming skb
> without ensuring the head was uncloned and hence writable.
>
> skb_cow_head has been used to ensure they are writable, however,
> this required some changes to error handling to ensure that
> if skb_cow_head failed it was not ignored.
>
> This really needs to be reviewed by someone who is more familiar
> with this code base to ensure any deallocation of skb's is
> still correct.
>
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>

You should also CC linux-wireless, otherwise patchwork won't see it.

-- 
Kalle Valo

^ permalink raw reply

* [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable
From: James Hughes @ 2017-04-20 11:16 UTC (permalink / raw)
  To: netdev, Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo
  Cc: James Hughes

The driver was adding header information to incoming skb
without ensuring the head was uncloned and hence writable.

skb_cow_head has been used to ensure they are writable, however,
this required some changes to error handling to ensure that
if skb_cow_head failed it was not ignored.

This really needs to be reviewed by someone who is more familiar
with this code base to ensure any deallocation of skb's is
still correct.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c    | 15 ++++++++--
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 23 +++++-----------
 .../broadcom/brcm80211/brcmfmac/fwsignal.c         | 32 +++++++++++++++++-----
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  7 ++++-
 4 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
index 038a960..b9d7d08 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
@@ -249,14 +249,19 @@ brcmf_proto_bcdc_set_dcmd(struct brcmf_pub *drvr, int ifidx, uint cmd,
 	return ret;
 }
 
-static void
+static int
 brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
 			 struct sk_buff *pktbuf)
 {
 	struct brcmf_proto_bcdc_header *h;
+	int err;
 
 	brcmf_dbg(BCDC, "Enter\n");
 
+	err = skb_cow_head(pktbuf, BCDC_HEADER_LEN);
+	if (err)
+		return err;
+
 	/* Push BDC header used to convey priority for buses that don't */
 	skb_push(pktbuf, BCDC_HEADER_LEN);
 
@@ -271,6 +276,8 @@ brcmf_proto_bcdc_hdrpush(struct brcmf_pub *drvr, int ifidx, u8 offset,
 	h->data_offset = offset;
 	BCDC_SET_IF_IDX(h, ifidx);
 	trace_brcmf_bcdchdr(pktbuf->data);
+
+	return 0;
 }
 
 static int
@@ -330,7 +337,11 @@ static int
 brcmf_proto_bcdc_txdata(struct brcmf_pub *drvr, int ifidx, u8 offset,
 			struct sk_buff *pktbuf)
 {
-	brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
+	int err = brcmf_proto_bcdc_hdrpush(drvr, ifidx, offset, pktbuf);
+
+	if (err)
+		return err;
+
 	return brcmf_bus_txdata(drvr->bus_if, pktbuf);
 }
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 5eaac13..08272e8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -198,7 +198,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 	int ret;
 	struct brcmf_if *ifp = netdev_priv(ndev);
 	struct brcmf_pub *drvr = ifp->drvr;
-	struct ethhdr *eh = (struct ethhdr *)(skb->data);
+	struct ethhdr *eh;
 
 	brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
 
@@ -212,23 +212,14 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 	}
 
 	/* Make sure there's enough room for any header */
-	if (skb_headroom(skb) < drvr->hdrlen) {
-		struct sk_buff *skb2;
-
-		brcmf_dbg(INFO, "%s: insufficient headroom\n",
-			  brcmf_ifname(ifp));
-		drvr->bus_if->tx_realloc++;
-		skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
-		dev_kfree_skb(skb);
-		skb = skb2;
-		if (skb == NULL) {
-			brcmf_err("%s: skb_realloc_headroom failed\n",
-				  brcmf_ifname(ifp));
-			ret = -ENOMEM;
-			goto done;
-		}
+	ret = skb_cow_head(skb, drvr->hdrlen);
+	if (ret) {
+		dev_kfree_skb_any(skb);
+		goto done;
 	}
 
+	eh = (struct ethhdr *)(skb->data);
+
 	/* validate length for ether packet */
 	if (skb->len < sizeof(*eh)) {
 		ret = -EINVAL;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
index a190f53..2510408 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
@@ -877,12 +877,15 @@ static void brcmf_fws_cleanup(struct brcmf_fws_info *fws, int ifidx)
 	brcmf_fws_hanger_cleanup(fws, matchfn, ifidx);
 }
 
-static u8 brcmf_fws_hdrpush(struct brcmf_fws_info *fws, struct sk_buff *skb)
+static int brcmf_fws_hdrpush(struct brcmf_fws_info *fws, struct sk_buff *skb,
+			     u8 *offset)
 {
 	struct brcmf_fws_mac_descriptor *entry = brcmf_skbcb(skb)->mac;
 	u8 *wlh;
 	u16 data_offset = 0;
 	u8 fillers;
+	int err;
+
 	__le32 pkttag = cpu_to_le32(brcmf_skbcb(skb)->htod);
 	__le16 pktseq = cpu_to_le16(brcmf_skbcb(skb)->htod_seq);
 
@@ -899,6 +902,11 @@ static u8 brcmf_fws_hdrpush(struct brcmf_fws_info *fws, struct sk_buff *skb)
 	fillers = round_up(data_offset, 4) - data_offset;
 	data_offset += fillers;
 
+	err = skb_cow_head(skb, data_offset);
+
+	if (err)
+		return err;
+
 	skb_push(skb, data_offset);
 	wlh = skb->data;
 
@@ -926,7 +934,9 @@ static u8 brcmf_fws_hdrpush(struct brcmf_fws_info *fws, struct sk_buff *skb)
 	if (fillers)
 		memset(wlh, BRCMF_FWS_TYPE_FILLER, fillers);
 
-	return (u8)(data_offset >> 2);
+	*offset = (u8)(data_offset >> 2);
+
+	return 0;
 }
 
 static bool brcmf_fws_tim_update(struct brcmf_fws_info *fws,
@@ -966,7 +976,8 @@ static bool brcmf_fws_tim_update(struct brcmf_fws_info *fws,
 		skcb->state = BRCMF_FWS_SKBSTATE_TIM;
 		skcb->htod = 0;
 		skcb->htod_seq = 0;
-		data_offset = brcmf_fws_hdrpush(fws, skb);
+		if (brcmf_fws_hdrpush(fws, skb, &data_offset))
+			return false;
 		ifidx = brcmf_skb_if_flags_get_field(skb, INDEX);
 		brcmf_fws_unlock(fws);
 		err = brcmf_proto_txdata(fws->drvr, ifidx, data_offset, skb);
@@ -1945,12 +1956,13 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb)
 		fws->stats.header_only_pkt++;
 }
 
-static u8 brcmf_fws_precommit_skb(struct brcmf_fws_info *fws, int fifo,
-				   struct sk_buff *p)
+static int brcmf_fws_precommit_skb(struct brcmf_fws_info *fws, int fifo,
+				   struct sk_buff *p, u8 *offset)
 {
 	struct brcmf_skbuff_cb *skcb = brcmf_skbcb(p);
 	struct brcmf_fws_mac_descriptor *entry = skcb->mac;
 	u8 flags;
+	int err;
 
 	if (skcb->state != BRCMF_FWS_SKBSTATE_SUPPRESSED)
 		brcmf_skb_htod_tag_set_field(p, GENERATION, entry->generation);
@@ -1963,7 +1975,10 @@ static u8 brcmf_fws_precommit_skb(struct brcmf_fws_info *fws, int fifo,
 		flags |= BRCMF_FWS_HTOD_FLAG_PKT_REQUESTED;
 	}
 	brcmf_skb_htod_tag_set_field(p, FLAGS, flags);
-	return brcmf_fws_hdrpush(fws, p);
+
+	err = brcmf_fws_hdrpush(fws, p, offset);
+
+	return err;
 }
 
 static void brcmf_fws_rollback_toq(struct brcmf_fws_info *fws,
@@ -2039,7 +2054,9 @@ static int brcmf_fws_commit_skb(struct brcmf_fws_info *fws, int fifo,
 	if (IS_ERR(entry))
 		return PTR_ERR(entry);
 
-	data_offset = brcmf_fws_precommit_skb(fws, fifo, skb);
+	if (!brcmf_fws_precommit_skb(fws, fifo, skb, &data_offset))
+		return PTR_ERR(entry);
+
 	entry->transit_count++;
 	if (entry->suppressed)
 		entry->suppr_transit_count++;
@@ -2100,6 +2117,7 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct sk_buff *skb)
 	int rc = 0;
 
 	brcmf_dbg(DATA, "tx proto=0x%X\n", ntohs(eh->h_proto));
+
 	/* determine the priority */
 	if ((skb->priority == 0) || (skb->priority > 7))
 		skb->priority = cfg80211_classify8021d(skb, NULL);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index d138260..0e53c8a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -2719,7 +2719,7 @@ static bool brcmf_sdio_prec_enq(struct pktq *q, struct sk_buff *pkt, int prec)
 
 static int brcmf_sdio_bus_txdata(struct device *dev, struct sk_buff *pkt)
 {
-	int ret = -EBADE;
+	int ret = -EBADE, err;
 	uint prec;
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
 	struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
@@ -2729,6 +2729,11 @@ static int brcmf_sdio_bus_txdata(struct device *dev, struct sk_buff *pkt)
 	if (sdiodev->state != BRCMF_SDIOD_DATA)
 		return -EIO;
 
+	err = skb_cow_head(pkt, bus->tx_hdrlen);
+
+	if (err)
+		return err;
+
 	/* Add space for the header */
 	skb_push(pkt, bus->tx_hdrlen);
 	/* precondition: IS_ALIGNED((unsigned long)(pkt->data), 2) */
-- 
2.9.3

^ permalink raw reply related

* Re: [PATCH net-next 2/3] net: hns: support deferred probe when no mdio
From: Matthias Brugger @ 2017-04-20 11:13 UTC (permalink / raw)
  To: netdev; +Cc: netdev, charles.chenxin, linuxarm
In-Reply-To: <1492510354-11300-3-git-send-email-yankejian@huawei.com>

On 18/04/17 12:12, Yankejian wrote:
> From: lipeng <lipeng321@huawei.com>
>
> In the hip06 and hip07 SoCs, phy connect to mdio bus.The mdio
> module is probed with module_init, and, as such,
> is not guaranteed to probe before the HNS driver. So we need
> to support deferred probe.
>
> We check for probe deferral in the mac init, so we not init DSAF
> when there is no mdio, and free all resource, to later learn that
> we need to defer the probe.
>
> Signed-off-by: lipeng <lipeng321@huawei.com>

on which kernel version is this patch based?
I checked against next-20170420 and it does not apply.


> ---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 34 +++++++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> index bdd8cdd..284ebfe 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> @@ -735,6 +735,8 @@ static int hns_mac_init_ex(struct hns_mac_cb *mac_cb)
>  	rc = phy_device_register(phy);
>  	if (rc) {
>  		phy_device_free(phy);
> +		dev_err(&mdio->dev, "registered phy fail at address %i\n",
> +			addr);
>  		return -ENODEV;
>  	}
>
> @@ -745,7 +747,7 @@ static int hns_mac_init_ex(struct hns_mac_cb *mac_cb)
>  	return 0;
>  }
>
> -static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
> +static int hns_mac_register_phy(struct hns_mac_cb *mac_cb)
>  {
>  	struct acpi_reference_args args;
>  	struct platform_device *pdev;
> @@ -755,24 +757,39 @@ static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
>
>  	/* Loop over the child nodes and register a phy_device for each one */
>  	if (!to_acpi_device_node(mac_cb->fw_port))
> -		return;
> +		return 0;

Please return appropriate errno.

>
>  	rc = acpi_node_get_property_reference(
>  			mac_cb->fw_port, "mdio-node", 0, &args);
>  	if (rc)
> -		return;
> +		return 0;

Please return appropriate errno.

>
>  	addr = hns_mac_phy_parse_addr(mac_cb->dev, mac_cb->fw_port);
>  	if (addr < 0)
> -		return;
> +		return 0;

Shouldn't we just error out here by returning addr?

>
>  	/* dev address in adev */
>  	pdev = hns_dsaf_find_platform_device(acpi_fwnode_handle(args.adev));
> +	if (!pdev) {
> +		dev_err(mac_cb->dev, "mac%d mdio pdev is NULL\n",
> +			mac_cb->mac_id);
> +		return 0;

Please return appropriate errno.

Regards,
Matthias

> +	}
> +
>  	mii_bus = platform_get_drvdata(pdev);
> +	if (!mii_bus) {
> +		dev_err(mac_cb->dev,
> +			"mac%d mdio is NULL, dsaf will probe again later\n",
> +			mac_cb->mac_id);
> +		return -EPROBE_DEFER;
> +	}
> +
>  	rc = hns_mac_register_phydev(mii_bus, mac_cb, addr);
>  	if (!rc)
>  		dev_dbg(mac_cb->dev, "mac%d register phy addr:%d\n",
>  			mac_cb->mac_id, addr);
> +
> +	return rc;
>  }
>
>  #define MAC_MEDIA_TYPE_MAX_LEN		16
> @@ -793,7 +810,7 @@ static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
>   *@np:device node
>   * return: 0 --success, negative --fail
>   */
> -static int  hns_mac_get_info(struct hns_mac_cb *mac_cb)
> +static int hns_mac_get_info(struct hns_mac_cb *mac_cb)
>  {
>  	struct device_node *np;
>  	struct regmap *syscon;
> @@ -903,7 +920,10 @@ static int  hns_mac_get_info(struct hns_mac_cb *mac_cb)
>  			}
>  		}
>  	} else if (is_acpi_node(mac_cb->fw_port)) {
> -		hns_mac_register_phy(mac_cb);
> +		ret = hns_mac_register_phy(mac_cb);
> +
> +		if (ret)
> +			return ret;
>  	} else {
>  		dev_err(mac_cb->dev, "mac%d cannot find phy node\n",
>  			mac_cb->mac_id);
> @@ -1065,6 +1085,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
>  			dsaf_dev->mac_cb[port_id] = mac_cb;
>  		}
>  	}
> +
>  	/* init mac_cb for all port */
>  	for (port_id = 0; port_id < max_port_num; port_id++) {
>  		mac_cb = dsaf_dev->mac_cb[port_id];
> @@ -1074,6 +1095,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
>  		ret = hns_mac_get_cfg(dsaf_dev, mac_cb);
>  		if (ret)
>  			return ret;
> +
>  		ret = hns_mac_init_ex(mac_cb);
>  		if (ret)
>  			return ret;
>

^ permalink raw reply

* [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt
From: Alexander Kochetkov @ 2017-04-20 11:00 UTC (permalink / raw)
  To: Florian Fainelli, netdev, linux-kernel, Sergei Shtylyov,
	Roger Quadros, Madalin Bucur
  Cc: Alexander Kochetkov

Hello Florian, Roger and Madalin!

This is slightly modified version of previous patch[1].

It take into account that phy_start_aneg() may be called from
differend places not only from PHY state machine. So the patch
use phy_trigger_machine() to schedule link state update
correctly.

I borrow some text from Roger's commit message.

Added 'Cc: stable <stable@vger.kernel.org> tag'
because the problem exist on 4.9 and 4.10 branches.

Don't know what commit brake the code, so I don't add
Fixes tag.

Roger, could you please test this one patch?

Madalin, I found, that you tested Roger’s patch[2] and this
one patch will get upstream instead of patch[2]. Could you
please test it?

Regards,
Alexander.

[1] http://patchwork.ozlabs.org/patch/743773/
[2] https://lkml.org/lkml/2017/3/30/517

Alexander Kochetkov (1):
  net: phy: fix auto-negotiation stall due to unavailable interrupt

 drivers/net/phy/phy.c |   40 ++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h   |    1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
1.7.9.5

^ permalink raw reply

* [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt
From: Alexander Kochetkov @ 2017-04-20 11:00 UTC (permalink / raw)
  To: Florian Fainelli, netdev, linux-kernel, Sergei Shtylyov,
	Roger Quadros, Madalin Bucur
  Cc: Alexander Kochetkov
In-Reply-To: <1492686004-30527-1-git-send-email-al.kochet@gmail.com>

The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
cable was plugged before the Ethernet interface was brought up.

The patch trigger PHY state machine to update link state if PHY was requested to
do auto-negotiation and auto-negotiation complete flag already set.

During power-up cycle the PHY do auto-negotiation, generate interrupt and set
auto-negotiation complete flag. Interrupt is handled by PHY state machine but
doesn't update link state because PHY is in PHY_READY state. After some time
MAC bring up, start and request PHY to do auto-negotiation. If there are no new
settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
PHY continue to stay in auto-negotiation complete state and doesn't fire
interrupt. At the same time PHY state machine expect that PHY started
auto-negotiation and is waiting for interrupt from PHY and it won't get it.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Cc: stable <stable@vger.kernel.org> # v4.9+
---
 drivers/net/phy/phy.c |   40 ++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h   |    1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..2d9975b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 EXPORT_SYMBOL(phy_mii_ioctl);
 
 /**
- * phy_start_aneg - start auto-negotiation for this PHY device
+ * phy_start_aneg_priv - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
+ * @sync: indicate whether we should wait for the workqueue cancelation
  *
  * Description: Sanitizes the settings (if we're not autonegotiating
  *   them), and then calls the driver's config_aneg function.
  *   If the PHYCONTROL Layer is operating, we change the state to
  *   reflect the beginning of Auto-negotiation or forcing.
  */
-int phy_start_aneg(struct phy_device *phydev)
+static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
 {
+	bool trigger = 0;
 	int err;
 
 	mutex_lock(&phydev->lock);
@@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
 		}
 	}
 
+	/* Re-schedule a PHY state machine to check PHY status because
+	 * negotiation may already be done and aneg interrupt may not be
+	 * generated.
+	 */
+	if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
+		err = phy_aneg_done(phydev);
+		if (err > 0) {
+			trigger = true;
+			err = 0;
+		}
+	}
+
 out_unlock:
 	mutex_unlock(&phydev->lock);
+
+	if (trigger)
+		phy_trigger_machine(phydev, sync);
+
 	return err;
 }
+
+/**
+ * phy_start_aneg - start auto-negotiation for this PHY device
+ * @phydev: the phy_device struct
+ *
+ * Description: Sanitizes the settings (if we're not autonegotiating
+ *   them), and then calls the driver's config_aneg function.
+ *   If the PHYCONTROL Layer is operating, we change the state to
+ *   reflect the beginning of Auto-negotiation or forcing.
+ */
+int phy_start_aneg(struct phy_device *phydev)
+{
+	return phy_start_aneg_priv(phydev, true);
+}
 EXPORT_SYMBOL(phy_start_aneg);
 
 /**
@@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
  *   state machine runs.
  */
 
-static void phy_trigger_machine(struct phy_device *phydev, bool sync)
+void phy_trigger_machine(struct phy_device *phydev, bool sync)
 {
 	if (sync)
 		cancel_delayed_work_sync(&phydev->state_queue);
@@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
 	mutex_unlock(&phydev->lock);
 
 	if (needs_aneg)
-		err = phy_start_aneg(phydev);
+		err = phy_start_aneg_priv(phydev, false);
 	else if (do_suspend)
 		phy_suspend(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7fc1105..b19ae66 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -840,6 +840,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n,
 void phy_mac_interrupt(struct phy_device *phydev, int new_link);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
+void phy_trigger_machine(struct phy_device *phydev, bool sync);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_ksettings_get(struct phy_device *phydev,
-- 
1.7.9.5

^ permalink raw reply related

* Re: [PATCH 11/16] esp: Use a synchronous crypto algorithm on offloading.
From: Steffen Klassert @ 2017-04-20 10:50 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, netdev
In-Reply-To: <20170420095235.GA8580@gondor.apana.org.au>

On Thu, Apr 20, 2017 at 05:52:35PM +0800, Herbert Xu wrote:
> On Thu, Apr 20, 2017 at 11:17:52AM +0200, Steffen Klassert wrote:
> >
> > I tried to use async algorithms but it lead to serveral problems.
> > The GSO layer can't handle async returns, we'd need callbacks
> > for all the GSO handlers. Also we need something where we can
> > requeue packets if the driver is busy etc.
> 
> Why would we need to requeue? As it is if you get an EBUSY on
> an IPsec packet it's simply dropped.

Yes we could do this, but the GSO problem remain.

We discussed this last year at netdevconf but could not come
up with an acceptable solutuion.

> 
> The main AES implementation on x86 is now async so I think it's
> pretty important that we support it out-of-the-box.

For now this is just a fallback to make hardware offloading
possible at all, so this is slowpath anyway. Allowing async
algorithms can (and should) be done in a second step once we
found a not too intrusive solution.

^ permalink raw reply

* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 10:42 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <20170419161736.GJ3357@nanopsycho.orion>

On 17-04-19 12:17 PM, Jiri Pirko wrote:
> Wed, Apr 19, 2017 at 06:07:48PM CEST, jhs@mojatatu.com wrote:
>> On 17-04-19 11:54 AM, Jiri Pirko wrote:
>>> Wed, Apr 19, 2017 at 05:37:15PM CEST, jhs@mojatatu.com wrote:
>>>> On 17-04-19 09:13 AM, Jiri Pirko wrote:
>>>>> Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@mojatatu.com wrote:
>>>>>> On 17-04-19 08:36 AM, Jiri Pirko wrote:
>>>>>>> Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote:
>>>>>>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>>>>
>>>>>>>> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>>>>>>>> net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
>>>>>>>> 3 files changed, 53 insertions(+), 12 deletions(-)
>>>>
>>>>>>>> +#define TCAA_MAX (__TCAA_MAX - 1)
>>>>>>>> #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>>>>>>>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>>>>>>>> -#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>>>>>>>> -#define TCAA_MAX 1
>>>>>>>> +#define TCA_ACT_TAB TCAA_ACT_TAB
>>>>>>>
>>>>>>> This is mess. What does "TCAA" stand for?
>>>>>>
>>>>>> TC Actions Attributes.  What would you call it? I could have
>>>>>> called it TCA_ROOT etc. But maybe a comment to just call it
>>>>>> TC Actions Attributes would be enough?
>>>>>
>>>>> TCA_DUMP_X
>>>>>
>>>>> it is only for dumping. Naming it "attribute" seems weird. Same as if
>>>>> you have: int variable_something;
>>>>>
>>>>
>>>> Jiri, this is not just for dumping. We are describing high level
>>>> attributes for tc actions.
>>>
>>> This is already present:
>>> enum {
>>>         TCA_ACT_UNSPEC,
>>>         TCA_ACT_KIND,
>>>         TCA_ACT_OPTIONS,
>>>         TCA_ACT_INDEX,
>>>         TCA_ACT_STATS,
>>>         TCA_ACT_PAD,
>>>         TCA_ACT_COOKIE,
>>>         __TCA_ACT_MAX
>>> };
>>>
>>> This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB)
>>>
>>> Looks like you are mixing these 2.
>>>
>>
>> No - That space is per action. The space i am defining is
>> above that in the the hierarchy. There used to be only
>> one attribute there (TCA_ACT_TAB) and now we are making
>> it more generic.
>
> Right. That's what I say. And that higher level is used only for
> dumping. That's why I suggested TCA_ACT_DUMP prefix.
>

DUMP is not right. _TAB is used for SET/DEL as well.
why dont we leave this alone so we can progress?
You can submit changes later if it bothers you still.

>
>>
>>>
>>>
>>
>>>> It is a _lot_ of code to change! Note:
>>>> This is all the UAPI visible code (the same coding style for 20 years).
>>>> I am worried about this part.
>>>
>>> We'll see. Lets do it in a sensitive way, in steps. But for new things,
>>> I think it is good not to stick with old and outlived habits.
>>>
>>>
>>
>> I know you have the muscle to get it done - so fine, i will start
>> with this one change.
>>
>>>
>>> Netlink is TLV, should be used as TLV. I don't see how you can run out
>>> any space. You tend to use Netlink in some weird hybrid mode, with only
>>> argument being space. I think that couple of bytes wasted is not
>>> a problem at all...
>>>
>>
>> You are not making sense to me still.
>> What you describe as "a couple of bytes" adds up when you have
>> a large number of objects. I am trying to cut down on data back
>> and forth from user/kernel and a bitmap is a well understood entity.
>
> The attributes in question are per-message, not per-object
>
>
>>
>> Even if i did use a TLV - when i am representing flags which require one
>> bit each - it makes sense to use a bitmask encapsulated in a TLV. Not
>> to waste a TLV per bit.
>
> That is the only correct way. For example, in future the kernel may
> report in extended ack that it does not support some specific attribute
> user passed. If you pack it all in one attr, that would not be possible.
> Also, what prevents user from passing random data on bit flag positions
> that you are not using? Current kernel would ignore it. Next kernel will
> do something different according to the flag bit. That is UAPI break.
> Essentialy the same thing what DaveM said about the struct. You have to
> define this completely, at the beginning, not possible to use the unused
> bits for other things in the future.
>


They are not the same issue Jiri. We have used bitmasks fine on netlink
message for a millenia. Nobody sets garbage on a bitmask they are not
supposed to touch. The struct padding thing is a shame the way it
turned out - now netlink can no longer have a claim to be a (good)
wire protocol.
Other thing: I know you feel strongly about this but I dont agree that
everything has to be a TLV and in no way, as a matter of principle, you 
are going to convince me  (even when the cows come home) that I have to
use 64 bits to carry a single bit just so I can use TLVs.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks
From: Stefan Hajnoczi @ 2017-04-20 10:32 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, Zhu Yanjun, Gerard Garcia, Jorgen Hansen
In-Reply-To: <20170413214410-mutt-send-email-mst@kernel.org>

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

On Thu, Apr 13, 2017 at 09:47:08PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 13, 2017 at 05:18:11PM +0100, Stefan Hajnoczi wrote:
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index af087b4..aae60c1 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/virtio_vsock.h>
> > +#include <uapi/linux/vsockmon.h>
> >  
> >  #include <net/sock.h>
> >  #include <net/af_vsock.h>
> > @@ -85,6 +86,63 @@ virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
> >  	return NULL;
> >  }
> >  
> > +/* Packet capture */
> > +void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > +	struct sk_buff *skb;
> > +	struct af_vsockmon_hdr *hdr;
> > +	unsigned char *t_hdr, *payload;
> > +
> > +	skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> > +			GFP_ATOMIC);
> > +	if (!skb)
> > +		return; /* nevermind if we cannot capture the packet */
> > +
> > +	hdr = (struct af_vsockmon_hdr *)skb_put(skb, sizeof(*hdr));
> > +
> > +	/* pkt->hdr is little-endian so no need to byteswap here */
> 
> Comment does not seem to make sense. Drop it?

All fields in pkt->hdr are little-endian.  All fields in the
af_vsockmon_hdr are little-endian too.

Normally code operating on either of these structs byteswaps the fields.
Therefore it seemed worth a comment to clarify that it's okay to omit
byteswaps.  The comment will help anyone auditing the code for
endianness bugs.

Why do you say it doesn't make sense?

> > +	hdr->src_cid = pkt->hdr.src_cid;
> > +	hdr->src_port = pkt->hdr.src_port;
> > +	hdr->dst_cid = pkt->hdr.dst_cid;
> > +	hdr->dst_port = pkt->hdr.dst_port;
> > +
> > +	hdr->transport = cpu_to_le16(AF_VSOCK_TRANSPORT_VIRTIO);
> > +	hdr->len = cpu_to_le16(sizeof(pkt->hdr));
> > +	hdr->reserved[0] = hdr->reserved[1] = 0;
> > +
> > +	switch(cpu_to_le16(pkt->hdr.op)) {
> 
> I'd expect this to be le16_to_cpu.
> Does this pass check build?

You are right, make C=2 warns about this and it should be le16_to_cpu().
I'll run check builds from now on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply

* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Jamal Hadi Salim @ 2017-04-20 10:28 UTC (permalink / raw)
  To: Cong Wang
  Cc: Wolfgang Bumiller, Linux Kernel Network Developers,
	David S. Miller, Lucas Bates, Roman Mashak
In-Reply-To: <CAM_iQpUPBnWKVHOi2SS6qoeHDPssdj=h03Wg1DLhKk7hbL6_Bg@mail.gmail.com>

On 17-04-19 01:25 PM, Cong Wang wrote:
> On Wed, Apr 19, 2017 at 4:32 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> This solves one issue, but I am afraid the issue Cong mentioned is a
>> possibility still.
>> Lets say user did a replace and tried to also replace the cookie
>> in that transaction. The init() succeeds but the cookie allocation
>> fails. To be correct we'll have to undo the replace i.e something
>> like uninit() which will restore back the old values.
>> This is very complex and unnecessary.
>
> It is not complex once we move to RCU completely, replacement
> is merely a pointer assignment, rollback is same.
>

Nice to hear.

> It is necessary too according to the rules of RCU, as I said before, the
> current code is broken, we can't modify an existing action with RCU,
> we have to make a copy. I do have plan to make actions truly RCU,
> but I have to redesign the action API's first.

Ok ;->
Lucas: Lets work to get those test suites in.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Jamal Hadi Salim @ 2017-04-20 10:24 UTC (permalink / raw)
  To: Wolfgang Bumiller, Cong Wang
  Cc: Linux Kernel Network Developers, David S. Miller
In-Reply-To: <1244443915.139.1492614192777@webmail.proxmox.com>

On 17-04-19 11:03 AM, Wolfgang Bumiller wrote:
>> On April 19, 2017 at 1:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>>
>> On 17-04-19 04:09 AM, Wolfgang Bumiller wrote:
>>
>> This solves one issue, but I am afraid the issue Cong mentioned is a
>> possibility still.
>> Lets say user did a replace and tried to also replace the cookie
>> in that transaction. The init() succeeds but the cookie allocation
>> fails. To be correct we'll have to undo the replace i.e something
>> like uninit() which will restore back the old values.
>> This is very complex and unnecessary.
>>
>> My suggestion:
>> If we move the cookie allocation before init we can save it and
>> only when init succeeds do we attach it to the action, otherwise
>> we free it on error path.
>
> Shouldn't the old code have freed an old a->act_cookie as well before
> replacing it then? nla_memdup_cookie() starts off by assigning a->act_cookie.
>
> (I've been running this loop for a while now:
> # while : ; do tc actions change action ok index 500 cookie $i; let i++; done
> and memory usage *seems* to be growing faster with the loop running - still
> slow, but visible. (I stopped most but not all background processes in this
> VM))
>
> I don't see the growth with the change below (replacing both patches).
> (although given the freeing of the old act_cookie pointer I wonder if
> this needs additional locking?)
>

I think we are safe. The cookie should not be touched by any datapath
code.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

^ permalink raw reply

* [PATCH] net: qrtr: potential use after free in qrtr_sendmsg()
From: Dan Carpenter @ 2017-04-20 10:21 UTC (permalink / raw)
  To: David S. Miller, Courtney Cavin
  Cc: Bjorn Andersson, David Ahern, Stephen Boyd, Johannes Berg, netdev,
	kernel-janitors

If skb_pad() fails then it frees the skb so we should check for errors.

Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index c36b0ec364a4..a9a8c7d5a4a9 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -658,7 +658,9 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	}
 
 	if (plen != len) {
-		skb_pad(skb, plen - len);
+		rc = skb_pad(skb, plen - len);
+		if (rc)
+			goto out_node;
 		skb_put(skb, plen - len);
 	}
 

^ permalink raw reply related

* Re: [PATCH block-tree] net: off by one in inet6_pton()
From: Sagi Grimberg @ 2017-04-20 10:19 UTC (permalink / raw)
  To: Dan Carpenter, David S. Miller, Jens Axboe
  Cc: Wei Tang, Alexey Dobriyan, netdev, linux-block, kernel-janitors
In-Reply-To: <20170413194231.GD591@mwanda>

Thanks Dan,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

^ permalink raw reply

* Re: [PATCH net] Fix net/hsr/hsr_device to check for freed skb buffer.
From: Sergei Shtylyov @ 2017-04-20 10:15 UTC (permalink / raw)
  To: Peter Heise, Arvid Brodin, David S. Miller; +Cc: netdev
In-Reply-To: <0102015b8a4332ea-1e523746-5fd0-48d9-9045-d9de973fcd1a-000000@eu-west-1.amazonses.com>

Hello!

On 4/20/2017 10:28 AM, Peter Heise wrote:

> Fixed an unchecked call of skb_put_padto. Return value was ignored
> before, however, skb_put_padto frees skb buffer in case of error.
>
> As reported by Dan Carpenter on kernel-janitors.
>
> Signed-off-by: Peter Heise <mail@pheise.de>
> ---
>  net/hsr/hsr_device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index c73160fb11e7..22d693f213be 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -314,7 +314,8 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  	hsr_sp = (typeof(hsr_sp)) skb_put(skb, sizeof(struct hsr_sup_payload));
>  	ether_addr_copy(hsr_sp->MacAddressA, master->dev->dev_addr);
>
> -	skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);
> +	if(skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))

    Need a space after *if*. Pleae run your patches thru scripts/checkpatch.pl 
before sending.

> +		return;
>
>  	hsr_forward_skb(skb, master);
>  	return;

MBR, Seregi

^ permalink raw reply

* [PATCH] usb: plusb: Add support for PL-27A1
From: Roman Spychała @ 2017-04-20 10:04 UTC (permalink / raw)
  To: linux-usb; +Cc: netdev, linux-kernel, Roman Spychała

From: Roman Spychała <roed@onet.eu>

This patch adds support for the PL-27A1 by adding the appropriate
USB ID's. This chip is used in the goobay Active USB 3.0 Data Link
and Unitek Y-3501 cables.

Signed-off-by: Roman Spychała <roed@onet.eu>
---
 drivers/net/usb/Kconfig |  2 +-
 drivers/net/usb/plusb.c | 15 +++++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 3dd490f53e48..f28bd74ac275 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -369,7 +369,7 @@ config USB_NET_NET1080
 	  optionally with LEDs that indicate traffic
 
 config USB_NET_PLUSB
-	tristate "Prolific PL-2301/2302/25A1 based cables"
+	tristate "Prolific PL-2301/2302/25A1/27A1 based cables"
 	# if the handshake/init/reset problems, from original 'plusb',
 	# are ever resolved ... then remove "experimental"
 	depends on USB_USBNET
diff --git a/drivers/net/usb/plusb.c b/drivers/net/usb/plusb.c
index 22e1a9a99a7d..6fe59373cba9 100644
--- a/drivers/net/usb/plusb.c
+++ b/drivers/net/usb/plusb.c
@@ -102,7 +102,7 @@ static int pl_reset(struct usbnet *dev)
 }
 
 static const struct driver_info	prolific_info = {
-	.description =	"Prolific PL-2301/PL-2302/PL-25A1",
+	.description =	"Prolific PL-2301/PL-2302/PL-25A1/PL-27A1",
 	.flags =	FLAG_POINTTOPOINT | FLAG_NO_SETINT,
 		/* some PL-2302 versions seem to fail usb_set_interface() */
 	.reset =	pl_reset,
@@ -139,6 +139,17 @@ static const struct usb_device_id	products [] = {
 					 * Host-to-Host Cable
 					 */
 	.driver_info =  (unsigned long) &prolific_info,
+
+},
+
+/* super speed cables */
+{
+	USB_DEVICE(0x067b, 0x27a1),     /* PL-27A1, no eeprom
+					 * also: goobay Active USB 3.0
+					 * Data Link,
+					 * Unitek Y-3501
+					 */
+	.driver_info =  (unsigned long) &prolific_info,
 },
 
 	{ },		// END
@@ -158,5 +169,5 @@ static struct usb_driver plusb_driver = {
 module_usb_driver(plusb_driver);
 
 MODULE_AUTHOR("David Brownell");
-MODULE_DESCRIPTION("Prolific PL-2301/2302/25A1 USB Host to Host Link Driver");
+MODULE_DESCRIPTION("Prolific PL-2301/2302/25A1/27A1 USB Host to Host Link Driver");
 MODULE_LICENSE("GPL");
-- 
2.12.2

^ permalink raw reply related

* RE: [net-next 04/14] i40e: dump VF information in debugfs
From: Mintz, Yuval @ 2017-04-20 10:09 UTC (permalink / raw)
  To: Or Gerlitz, Jeff Kirsher, Mitch Williams, David Miller
  Cc: Linux Netdev List, nhorman@redhat.com, sassmann@redhat.com,
	jogreene@redhat.com
In-Reply-To: <CAJ3xEMiCuV5A_Ccy7=mKSjc9xxhg4enUxDq-MM-cO3YjSa439A@mail.gmail.com>

> > Dump some internal state about VFs through debugfs. This provides
> > information not available with 'ip link show'.
> 
> such as?
> 
> donnwantobethedebugfspolice, but still, in the 2-3 times we tried to push
> debugfs to MLNX NIC drivers, Dave disallowed that, and lately the switch
> team even went further and deleted that portion of the mlxsw driver -- all to
> all,  I don't see much point for these type of changes, thoughts?

Don't want to hikjack your thread, but continuing this topic -
Is there some flat-out disapproval for debugfs in net-next now?

We're currently internally engaged with adding qed support for register dumps
[~equivalents for `ethtool -d' outputs] through debugfs, on behalf of storage
drivers [qedi/qedf] lacking the API for doing that.


^ permalink raw reply

* Re: [PATCH 11/16] esp: Use a synchronous crypto algorithm on offloading.
From: Herbert Xu @ 2017-04-20  9:52 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: David Miller, netdev
In-Reply-To: <20170420091752.GH2649@secunet.com>

On Thu, Apr 20, 2017 at 11:17:52AM +0200, Steffen Klassert wrote:
>
> I tried to use async algorithms but it lead to serveral problems.
> The GSO layer can't handle async returns, we'd need callbacks
> for all the GSO handlers. Also we need something where we can
> requeue packets if the driver is busy etc.

Why would we need to requeue? As it is if you get an EBUSY on
an IPsec packet it's simply dropped.

The main AES implementation on x86 is now async so I think it's
pretty important that we support it out-of-the-box.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* Re: [PATCH net] xfrm: calculate L4 checksums also for GSO case before encrypting packets
From: Steffen Klassert @ 2017-04-20  9:47 UTC (permalink / raw)
  To: Ansis Atteka; +Cc: Ansis Atteka, netdev
In-Reply-To: <CAMQa7Bimd378QJgsv6yBP0ULH+__C9gjjcXa39wx1z7p_9mT=g@mail.gmail.com>

On Tue, Apr 18, 2017 at 07:10:03PM -0700, Ansis Atteka wrote:
> 
> However, after taking pointers from your patch I came up with this one
> that may solve this problem once and for all (note, that I was seeing
> this bug only with ixgbe NIC that supports tx csum offloads). I hope
> it does not break any other IPsec tests that you have.
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index b2be1d9..7812501 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -29,6 +29,7 @@ static struct sk_buff
> *__skb_udp_tunnel_segment(struct sk_buff *skb,
>         u16 mac_len = skb->mac_len;
>         int udp_offset, outer_hlen;
>         __wsum partial;
> +       bool need_ipsec;
> 
>         if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
>                 goto out;
> @@ -62,8 +63,10 @@ static struct sk_buff
> *__skb_udp_tunnel_segment(struct sk_buff *skb,
> 
>         ufo = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
> 
> +       need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
>         /* Try to offload checksum if possible */
>         offload_csum = !!(need_csum &&
> +                         !need_ipsec &&
>                           (skb->dev->features &
>                            (is_ipv6 ? (NETIF_F_HW_CSUM | NETIF_F_IPV6_CSUM) :
>                                       (NETIF_F_HW_CSUM | NETIF_F_IP_CSUM))));

This looks good, but we should fix udp4_ufo_fragment() too.

Thanks!

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox