Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Cong Wang @ 2014-11-19 22:26 UTC (permalink / raw)
  To: Wengang; +Cc: Eric Dumazet, netdev
In-Reply-To: <546C4022.5010509@oracle.com>

On Tue, Nov 18, 2014 at 11:00 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>
> Yes, that's true. So the simplest way is move ipoib_header_ops to vmlinux.
>

That is not an option. Perhaps you need RCU to protect the dev->header_ops
pointer.

^ permalink raw reply

* Re: [PATCH v4] ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Arnd Bergmann @ 2014-11-19 22:12 UTC (permalink / raw)
  To: Jiri Bohac; +Cc: David Miller, acme, netdev
In-Reply-To: <20141119220549.GA26133@midget.suse.cz>

On Wednesday 19 November 2014 23:05:49 Jiri Bohac wrote:
> This fixes an old regression introduced by commit
> b0d0d915 (ipx: remove the BKL).
> 
> When a recvmsg syscall blocks waiting for new data, no data can be sent on the
> same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
> 
> This breaks mars-nwe (NetWare emulator):
> - the ncpserv process reads the request using recvmsg
> - ncpserv forks and spawns nwconn
> - ncpserv calls a (blocking) recvmsg and waits for new requests
> - nwconn deadlocks in sendmsg on the same socket 
> 
> Commit b0d0d915 has simply replaced BKL locking with
> lock_sock/release_sock. Unlike now, BKL got unlocked while
> sleeping, so a blocking recvmsg did not block a concurrent
> sendmsg.
> 
> Only keep the socket locked while actually working with the socket data and
> release it prior to calling skb_recv_datagram(). 
> 
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> 

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* [PATCH v4] ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Jiri Bohac @ 2014-11-19 22:05 UTC (permalink / raw)
  To: David Miller; +Cc: jbohac, arnd, acme, netdev
In-Reply-To: <20141119.154400.1045032776950540216.davem@davemloft.net>

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the
same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket 

Commit b0d0d915 has simply replaced BKL locking with
lock_sock/release_sock. Unlike now, BKL got unlocked while
sleeping, so a blocking recvmsg did not block a concurrent
sendmsg.

Only keep the socket locked while actually working with the socket data and
release it prior to calling skb_recv_datagram(). 


Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index a0c7536..d0725d9 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 	struct ipxhdr *ipx = NULL;
 	struct sk_buff *skb;
 	int copied, rc;
+	bool locked = true;
 
 	lock_sock(sk);
 	/* put the autobinding in */
@@ -1790,6 +1791,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 	if (sock_flag(sk, SOCK_ZAPPED))
 		goto out;
 
+	release_sock(sk);
+	locked = false;
 	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
 				flags & MSG_DONTWAIT, &rc);
 	if (!skb) {
@@ -1825,7 +1828,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 out_free:
 	skb_free_datagram(sk, skb);
 out:
-	release_sock(sk);
+	if (locked)
+		release_sock(sk);
 	return rc;
 }
 
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

^ permalink raw reply related

* Re: [PATCHv3 net] openvswitch: Don't validate IPv6 label masks.
From: Pravin Shelar @ 2014-11-19 22:05 UTC (permalink / raw)
  To: Joe Stringer; +Cc: netdev, LKML, dev@openvswitch.org
In-Reply-To: <1416434089-47062-1-git-send-email-joestringer@nicira.com>

On Wed, Nov 19, 2014 at 1:54 PM, Joe Stringer <joestringer@nicira.com> wrote:
> When userspace doesn't provide a mask, OVS datapath generates a fully
> unwildcarded mask for the flow by copying the flow and setting all bits
> in all fields. For IPv6 label, this creates a mask that matches on the
> upper 12 bits, causing the following error:
>
> openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)
>
> This patch ignores the label validation check for masks, avoiding this
> error.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Thanks for the fix.
Acked-by: Pravin B Shelar <pshelar@nicira.com>

> ---
> v3: Alternative approach.
>     Was "openvswitch: Fix mask generation for IPv6 labels."
> v2: OR lower 20 bits (upper 12 bits remain from earlier memdup)
> ---
>  net/openvswitch/flow_netlink.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index fa4ec2e..089b195 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -690,7 +690,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>                         return -EINVAL;
>                 }
>
> -               if (ipv6_key->ipv6_label & htonl(0xFFF00000)) {
> +               if (!is_mask && ipv6_key->ipv6_label & htonl(0xFFF00000)) {
>                         OVS_NLERR("IPv6 flow label %x is out of range (max=%x).\n",
>                                   ntohl(ipv6_key->ipv6_label), (1 << 20) - 1);
>                         return -EINVAL;
> --
> 1.7.10.4
>

^ permalink raw reply

* Re: [PATCH net-next] sky2: use new netdev_rss_key_fill() helper
From: David Miller @ 2014-11-19 22:04 UTC (permalink / raw)
  To: ipm; +Cc: netdev, mlindner, stephen, edumazet
In-Reply-To: <1416388011-10491-1-git-send-email-ipm@chirality.org.uk>

From: Ian Morris <ipm@chirality.org.uk>
Date: Wed, 19 Nov 2014 09:06:51 +0000

> Switch to a random RSS key rather than a fixed one.
> Using netdev_rss_key_fill helper also ensures that all ports share
> a common key.
> 
> See also commit 960fb622f85180f36d3aff82af53e2be3db2f888.
> 
> Signed-off-by: Ian Morris <ipm@chirality.org.uk>

Applied, thank you.

^ permalink raw reply

* Re: [PATCH net-next v2] enic: support skb->xmit_more
From: David Miller @ 2014-11-19 22:02 UTC (permalink / raw)
  To: _govind; +Cc: netdev, ssujith, benve, eric.dumazet
In-Reply-To: <1416382172-28775-1-git-send-email-_govind@gmx.com>

From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Wed, 19 Nov 2014 12:59:32 +0530

> Check and update posted_index only when skb->xmit_more is 0 or tx queue is full.
> 
> v2:
> use txq_map instead of skb_get_queue_mapping(skb)
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH net] cxgb4i : Don't block unload/cxgb4 unload when remote closes TCP connection
From: David Miller @ 2014-11-19 21:59 UTC (permalink / raw)
  To: anish; +Cc: netdev, linux-scsi, hch, jbottomley, kxie, manojmalviya
In-Reply-To: <1416366591-9699-1-git-send-email-anish@chelsio.com>

From: Anish Bhatt <anish@chelsio.com>
Date: Tue, 18 Nov 2014 19:09:51 -0800

> cxgb4i was returning wrong error and not releasing module reference if remote
> end abruptly closed TCP connection. This prevents the cxgb4 network module from
> being unloaded, further affecting other network drivers dependent on cxgb4
> 
> Sending to net as this affects all cxgb4 based network drivers.
> 
> Signed-off-by: Anish Bhatt <anish@chelsio.com>

Applied, thank you.

^ permalink raw reply

* Re: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
From: Jeff Kirsher @ 2014-11-19 21:56 UTC (permalink / raw)
  To: Tantilov, Emil S
  Cc: Vlad Yasevich, netdev@vger.kernel.org, Keller, Jacob E,
	Skidmore, Donald C
In-Reply-To: <87618083B2453E4A8714035B62D67992501A1011@FMSMSX105.amr.corp.intel.com>

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

On Wed, 2014-11-19 at 13:50 -0800, Tantilov, Emil S wrote:
> >-----Original Message-----
> >From: Vlad Yasevich [mailto:vyasevich@gmail.com]
> >Sent: Wednesday, November 19, 2014 11:41 AM
> >To: Tantilov, Emil S; netdev@vger.kernel.org
> >Cc: Kirsher, Jeffrey T; Keller, Jacob E; Skidmore, Donald C
> >Subject: Re: [PATCH] ixgbe: Correctly disable vlan filter in
> >promiscuous mode
> >
> >On 11/18/2014 03:24 PM, Tantilov, Emil S wrote:
> >>> -----Original Message-----
> >>> From: netdev-owner@vger.kernel.org [mailto:netdev-
> >>> owner@vger.kernel.org] On Behalf Of Vladislav Yasevich
> >>> Sent: Tuesday, November 18, 2014 11:28 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: Vladislav Yasevich; Kirsher, Jeffrey T; Keller, Jacob E
> >>> Subject: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
> >>>
> >>> IXGBE adapater seems to require that vlan filtering be enabled if VMDQ
> >>> or SRIOV are enabled.  When those functions are disabled,
> >>> vlan filtering may be disabled in promiscuous mode.
> >>>
> >>> Prior to commit a9b8943ee129e11045862d6d6e25c5b63c95403c
> >>>    ixgbe: remove vlan_filter_disable and enable functions
> >>>
> >>> the logic was correct.  However, after the commit the logic
> >>> got reversed and vlan filtered in now turned on when VMDQ/SRIOV
> >>> is disabled.
> >>>
> >>> This patch changes the condition to enable hw vlan filtered
> >>> when VMDQ or SRIOV is enabled.
> >>>
> >>> Fixes: a9b8943ee129e11045862d6d6e25c5b63c95403c (ixgbe:
> >>> remove
> >>> vlan_filter_disable and enable functions)
> >>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >>> CC: Jacob Keller <jacob.e.keller@intel.com>
> >>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>> ---
> >>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> index d2df4e3..3f81c7a 100644
> >>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >>> @@ -3936,8 +3936,8 @@ void ixgbe_set_rx_mode(struct
> >>> net_device *netdev)
> >>> 		 * if SR-IOV and VMDQ are disabled - otherwise ensure
> >>> 		 * that hardware VLAN filters remain enabled.
> >>> 		 */
> >>> -		if (!(adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
> >>> -					IXGBE_FLAG_SRIOV_ENABLED)))
> >>> +		if (adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
> >>> +				      IXGBE_FLAG_SRIOV_ENABLED))
> >>> 			vlnctrl |= (IXGBE_VLNCTRL_VFE |
> >>> IXGBE_VLNCTRL_CFIEN);
> >>> 	} else {
> >>> 		if (netdev->flags & IFF_ALLMULTI) {
> >>
> >> The current logic is correct and it's like this on purpose
> >> as it should be obvious by the comment preceding this check.
> >
> >Actually the comment right now does not match what the code
> >is doing.
> >
> >The comment states:
> >                /* Only disable hardware filter vlans in promiscuous mode
> >                 * if SR-IOV and VMDQ are disabled - otherwise ensure
> >                 * that hardware VLAN filters remain enabled.
> >                 */
> >
> >However, the code currently will _enable_ vlan filtering if VMDQ/SRIOV
> >is _disabled_ in promiscuous mode.
> 
> Actually you're right. Sorry - I misread the patch initially.
> This is indeed a bug in the code.
> 
> Acked-by: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Jeff should pick it up.

Consider it done, I have added Vlad's patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* Re: [PATCH net-next] bpf: fix arraymap NULL deref and missing overflow and zero size checks
From: David Miller @ 2014-11-19 21:55 UTC (permalink / raw)
  To: ast; +Cc: fengguang.wu, hannes, dborkman, netdev, linux-kernel
In-Reply-To: <1416360736-9531-1-git-send-email-ast@plumgrid.com>

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 18 Nov 2014 17:32:16 -0800

> - fix NULL pointer dereference:
> kernel/bpf/arraymap.c:41 array_map_alloc() error: potential null dereference 'array'.  (kzalloc returns null)
> kernel/bpf/arraymap.c:41 array_map_alloc() error: we previously assumed 'array' could be null (see line 40)
> 
> - integer overflow check was missing in arraymap
> (hashmap checks for overflow via kmalloc_array())
> 
> - arraymap can round_up(value_size, 8) to zero. check was missing.
> 
> - hashmap was missing zero size check as well, since roundup_pow_of_two() can
> truncate into zero
> 
> - found a typo in the arraymap comment and unnecessary empty line
> 
> Fix all of these issues and make both overflow checks explicit U32 in size.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> This silly NULL deref bug and missing overflow check was an oversight when
> I refactored the code from two allocations (kmalloc for struct bpf_array and
> kcalloc for array of elements) in the first implementation of arraymap
> into one allocation which is this code.

Applied, thanks.

^ permalink raw reply

* Re: [PATCH v2] ipv6: delete protocol and unregister rtnetlink when cleanup
From: David Miller @ 2014-11-19 21:55 UTC (permalink / raw)
  To: duanj.fnst; +Cc: cwang, netdev, eric.dumazet
In-Reply-To: <546BF3EB.1080603@cn.fujitsu.com>

From: Duan Jiong <duanj.fnst@cn.fujitsu.com>
Date: Wed, 19 Nov 2014 09:35:39 +0800

> pim6_protocol was added when initiation, but it not deleted.
> Similarly, unregister RTNL_FAMILY_IP6MR rtnetlink.
> 
> Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> ---
> v2: add missing rtnl_unregister()

Applied, thanks.

^ permalink raw reply

* [PATCHv3 net] openvswitch: Don't validate IPv6 label masks.
From: Joe Stringer @ 2014-11-19 21:54 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA

When userspace doesn't provide a mask, OVS datapath generates a fully
unwildcarded mask for the flow by copying the flow and setting all bits
in all fields. For IPv6 label, this creates a mask that matches on the
upper 12 bits, causing the following error:

openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)

This patch ignores the label validation check for masks, avoiding this
error.

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v3: Alternative approach.
    Was "openvswitch: Fix mask generation for IPv6 labels."
v2: OR lower 20 bits (upper 12 bits remain from earlier memdup)
---
 net/openvswitch/flow_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fa4ec2e..089b195 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -690,7 +690,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 			return -EINVAL;
 		}
 
-		if (ipv6_key->ipv6_label & htonl(0xFFF00000)) {
+		if (!is_mask && ipv6_key->ipv6_label & htonl(0xFFF00000)) {
 			OVS_NLERR("IPv6 flow label %x is out of range (max=%x).\n",
 				  ntohl(ipv6_key->ipv6_label), (1 << 20) - 1);
 			return -EINVAL;
-- 
1.7.10.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply related

* Re: [PATCH 1/1] mISDN: Deletion of unnecessary checks before the function call "vfree"
From: David Miller @ 2014-11-19 21:54 UTC (permalink / raw)
  To: elfring; +Cc: isdn, netdev, linux-kernel, kernel-janitors, julia.lawall
In-Reply-To: <546CF5A6.3030305@users.sourceforge.net>

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Nov 2014 20:55:18 +0100

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 19 Nov 2014 20:48:26 +0100
> 
> The vfree() function performs also input parameter validation. Thus the test
> around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied, thanks.

^ permalink raw reply

* Re: [RFC] situation with csum_and_copy_... API
From: David Miller @ 2014-11-19 21:53 UTC (permalink / raw)
  To: viro; +Cc: torvalds, netdev, linux-kernel
In-Reply-To: <20141119213006.GE7996@ZenIV.linux.org.uk>

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Wed, 19 Nov 2014 21:30:07 +0000

> On Wed, Nov 19, 2014 at 04:17:44PM -0500, David Miller wrote:
>> > Seeing a "__get_user()" and just being able to glance up in the same
>> > function and seeing the "access_ok()" is just a good safety net. And
>> > means that people don't have to waste time thinking about or looking
>> > for where the hell the security net really is.
>> 
>> Fair enough.
> 
> OK, with 3/5 dropped 4/5 get a trivial conflict (removal of function in 4/5
> vs. change in it in 3/5).  With that dealt with, the sucker is in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem
> 
> Shortlog:
> Al Viro (4):
>       separate kernel- and userland-side msghdr
>       {compat_,}verify_iovec(): switch to generic copying of iovecs
>       fold verify_iovec() into copy_msghdr_from_user()
>       bury skb_copy_to_page()

Pulled, thanks Al.

^ permalink raw reply

* Re: [PATCHv2 net] openvswitch: Fix mask generation for IPv6 labels.
From: Joe Stringer @ 2014-11-19 21:52 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, pshelar, dev
In-Reply-To: <1416432001-35678-1-git-send-email-joestringer@nicira.com>

On Wednesday, November 19, 2014 13:20:01 Joe Stringer wrote:
> When userspace doesn't provide a mask, OVS datapath generates a fully
> unwildcarded mask for the flow. This is done by taking a copy of the
> flow key, then iterating across its attributes, setting all values to
> 0xff. This works for most attributes, as the length of the netlink
> attribute typically matches the length of the value. However, IPv6
> labels only use the lower 20 bits of the field. This patch makes a
> special case to handle this.
> 
> This fixes the following error seen when installing IPv6 flows without a
> mask:
> 
> openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
> max=fffff)

Based on offlist discussion with Pravin, I'll send a patch for an alternative 
approach.

^ permalink raw reply

* RE: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
From: Tantilov, Emil S @ 2014-11-19 21:50 UTC (permalink / raw)
  To: Vlad Yasevich, netdev@vger.kernel.org
  Cc: Kirsher, Jeffrey T, Keller, Jacob E, Skidmore, Donald C
In-Reply-To: <546CF258.9050205@gmail.com>

>-----Original Message-----
>From: Vlad Yasevich [mailto:vyasevich@gmail.com]
>Sent: Wednesday, November 19, 2014 11:41 AM
>To: Tantilov, Emil S; netdev@vger.kernel.org
>Cc: Kirsher, Jeffrey T; Keller, Jacob E; Skidmore, Donald C
>Subject: Re: [PATCH] ixgbe: Correctly disable vlan filter in
>promiscuous mode
>
>On 11/18/2014 03:24 PM, Tantilov, Emil S wrote:
>>> -----Original Message-----
>>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>>> owner@vger.kernel.org] On Behalf Of Vladislav Yasevich
>>> Sent: Tuesday, November 18, 2014 11:28 AM
>>> To: netdev@vger.kernel.org
>>> Cc: Vladislav Yasevich; Kirsher, Jeffrey T; Keller, Jacob E
>>> Subject: [PATCH] ixgbe: Correctly disable vlan filter in promiscuous mode
>>>
>>> IXGBE adapater seems to require that vlan filtering be enabled if VMDQ
>>> or SRIOV are enabled.  When those functions are disabled,
>>> vlan filtering may be disabled in promiscuous mode.
>>>
>>> Prior to commit a9b8943ee129e11045862d6d6e25c5b63c95403c
>>>    ixgbe: remove vlan_filter_disable and enable functions
>>>
>>> the logic was correct.  However, after the commit the logic
>>> got reversed and vlan filtered in now turned on when VMDQ/SRIOV
>>> is disabled.
>>>
>>> This patch changes the condition to enable hw vlan filtered
>>> when VMDQ or SRIOV is enabled.
>>>
>>> Fixes: a9b8943ee129e11045862d6d6e25c5b63c95403c (ixgbe:
>>> remove
>>> vlan_filter_disable and enable functions)
>>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> CC: Jacob Keller <jacob.e.keller@intel.com>
>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index d2df4e3..3f81c7a 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -3936,8 +3936,8 @@ void ixgbe_set_rx_mode(struct
>>> net_device *netdev)
>>> 		 * if SR-IOV and VMDQ are disabled - otherwise ensure
>>> 		 * that hardware VLAN filters remain enabled.
>>> 		 */
>>> -		if (!(adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
>>> -					IXGBE_FLAG_SRIOV_ENABLED)))
>>> +		if (adapter->flags & (IXGBE_FLAG_VMDQ_ENABLED |
>>> +				      IXGBE_FLAG_SRIOV_ENABLED))
>>> 			vlnctrl |= (IXGBE_VLNCTRL_VFE |
>>> IXGBE_VLNCTRL_CFIEN);
>>> 	} else {
>>> 		if (netdev->flags & IFF_ALLMULTI) {
>>
>> The current logic is correct and it's like this on purpose
>> as it should be obvious by the comment preceding this check.
>
>Actually the comment right now does not match what the code
>is doing.
>
>The comment states:
>                /* Only disable hardware filter vlans in promiscuous mode
>                 * if SR-IOV and VMDQ are disabled - otherwise ensure
>                 * that hardware VLAN filters remain enabled.
>                 */
>
>However, the code currently will _enable_ vlan filtering if VMDQ/SRIOV
>is _disabled_ in promiscuous mode.

Actually you're right. Sorry - I misread the patch initially.
This is indeed a bug in the code.

Acked-by: Emil Tantilov <emil.s.tantilov@intel.com>

Jeff should pick it up.

Thanks,
Emil

^ permalink raw reply

* Re: [RFC] situation with csum_and_copy_... API
From: Al Viro @ 2014-11-19 21:30 UTC (permalink / raw)
  To: David Miller; +Cc: torvalds, netdev, linux-kernel
In-Reply-To: <20141119.161744.1661940121298888832.davem@davemloft.net>

On Wed, Nov 19, 2014 at 04:17:44PM -0500, David Miller wrote:
> > Seeing a "__get_user()" and just being able to glance up in the same
> > function and seeing the "access_ok()" is just a good safety net. And
> > means that people don't have to waste time thinking about or looking
> > for where the hell the security net really is.
> 
> Fair enough.

OK, with 3/5 dropped 4/5 get a trivial conflict (removal of function in 4/5
vs. change in it in 3/5).  With that dealt with, the sucker is in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem

Shortlog:
Al Viro (4):
      separate kernel- and userland-side msghdr
      {compat_,}verify_iovec(): switch to generic copying of iovecs
      fold verify_iovec() into copy_msghdr_from_user()
      bury skb_copy_to_page()

Diffstat:
 arch/arm/kernel/sys_oabi-compat.c |    4 +--
 include/linux/socket.h            |   17 +++++++---
 include/linux/syscalls.h          |    6 ++--
 include/net/compat.h              |    5 ++-
 include/net/sock.h                |   23 -------------
 net/compat.c                      |   83 +++++++++++++++-------------------------------
 net/core/iovec.c                  |   47 --------------------------
 net/socket.c                      |  140 +++++++++++++++++++++++++++++++++++++-----------------------------------------
 8 files changed, 114 insertions(+), 211 deletions(-)

I'll post more for review after I finally get some sleep - up for bloody 27
hours by now ;-/

^ permalink raw reply

* Re: Question about Patch Submissions
From: rapier @ 2014-11-19 21:29 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: Cong Wang, netdev
In-Reply-To: <CAADnVQ+1_Ct3hhmZrCqpxcT=-XS4OXrpRbrHHHyOKQOuoDc06g@mail.gmail.com>

On 11/18/14, 8:38 PM, Alexei Starovoitov wrote:
> On Tue, Nov 18, 2014 at 12:39 PM, rapier <rapier@psc.edu> wrote:
>> break this down for presentation. We can make sure each section of the patch
>> applies cleanly but the entirety of the patch set would have to be applied
>> for it to compile.
>
> all patches need to compile cleanly on all architectures,
> otherwise bisect will be broken.

The issue is that this is large patch set. The diff comes in at around 
2200 lines and adds new functionality to the kernel by implementing RFC 
4898. What I'm really trying to figure out is how to split up the diff 
files so I'm not dropping all 2200 lines into a single email message. 
I'm assuming that's not really how people want to see submissions but if 
I am incorrect please let me know.

There are two major components that we should be able to submit as 
distinct patches so that should reduce the line count per submission.

Chris Rapier

^ permalink raw reply

* [PATCHv2 net] openvswitch: Fix mask generation for IPv6 labels.
From: Joe Stringer @ 2014-11-19 21:20 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA

When userspace doesn't provide a mask, OVS datapath generates a fully
unwildcarded mask for the flow. This is done by taking a copy of the
flow key, then iterating across its attributes, setting all values to
0xff. This works for most attributes, as the length of the netlink
attribute typically matches the length of the value. However, IPv6
labels only use the lower 20 bits of the field. This patch makes a
special case to handle this.

This fixes the following error seen when installing IPv6 flows without a mask:

openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff, max=fffff)

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v2: OR lower 20 bits (upper 12 bits remain from earlier memdup)
---
 net/openvswitch/flow_netlink.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fa4ec2e..e530025 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -825,7 +825,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 	return 0;
 }
 
-static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
+static void mask_set_nlattr(struct nlattr *attr)
 {
 	struct nlattr *nla;
 	int rem;
@@ -835,16 +835,18 @@ static void nlattr_set(struct nlattr *attr, u8 val, bool is_attr_mask_key)
 		/* We assume that ovs_key_lens[type] == -1 means that type is a
 		 * nested attribute
 		 */
-		if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
-			nlattr_set(nla, val, false);
+		if (ovs_key_lens[nla_type(nla)] == -1)
+			nla_for_each_nested(nla, attr, rem)
+				memset(nla_data(nla), 0xff, nla_len(nla));
 		else
-			memset(nla_data(nla), val, nla_len(nla));
-	}
-}
+			memset(nla_data(nla), 0xff, nla_len(nla));
 
-static void mask_set_nlattr(struct nlattr *attr, u8 val)
-{
-	nlattr_set(attr, val, true);
+		if (nla_type(nla) == OVS_KEY_ATTR_IPV6) {
+			struct ovs_key_ipv6 *ipv6_key = nla_data(nla);
+
+			ipv6_key->ipv6_label |= htonl(0x000FFFFF);
+		}
+	}
 }
 
 /**
@@ -926,7 +928,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 		if (!newmask)
 			return -ENOMEM;
 
-		mask_set_nlattr(newmask, 0xff);
+		mask_set_nlattr(newmask);
 
 		/* The userspace does not send tunnel attributes that are 0,
 		 * but we should not wildcard them nonetheless.
-- 
1.7.10.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply related

* Re: [RFC] situation with csum_and_copy_... API
From: David Miller @ 2014-11-19 21:17 UTC (permalink / raw)
  To: torvalds; +Cc: viro, netdev, linux-kernel
In-Reply-To: <CA+55aFyQR2gOzEDROcWFcQzLvTjOxyJRjFJXJ03JB5knd-Gsgg@mail.gmail.com>

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 19 Nov 2014 12:40:53 -0800

> On Wed, Nov 19, 2014 at 12:31 PM, David Miller <davem@davemloft.net> wrote:
>>
>> But that is just my opinion, and yes I do acknowledge that we've had
>> serious holes in this area in the past.
> 
> The serious holes have generally been exactly in the "upper layers
> already check" camp, and then it turns out that some odd ioctl or
> other thing ends up doing something odd and interesting.
> 
> If Al has actual performance profiles showing that the access_ok() is
> a real problem, then fine. As a low-level optimization, I agree with
> it. But not as a "let's just drop them, and make the security rules be
> non-local and subtle, and require people to know the details of the
> whole call-chain".
> 
> Seeing a "__get_user()" and just being able to glance up in the same
> function and seeing the "access_ok()" is just a good safety net. And
> means that people don't have to waste time thinking about or looking
> for where the hell the security net really is.

Fair enough.

^ permalink raw reply

* Re: [RFC] situation with csum_and_copy_... API
From: Al Viro @ 2014-11-19 21:17 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Miller, Network Development, Linux Kernel Mailing List
In-Reply-To: <CA+55aFyQR2gOzEDROcWFcQzLvTjOxyJRjFJXJ03JB5knd-Gsgg@mail.gmail.com>

On Wed, Nov 19, 2014 at 12:40:53PM -0800, Linus Torvalds wrote:
> On Wed, Nov 19, 2014 at 12:31 PM, David Miller <davem@davemloft.net> wrote:
> >
> > But that is just my opinion, and yes I do acknowledge that we've had
> > serious holes in this area in the past.
> 
> The serious holes have generally been exactly in the "upper layers
> already check" camp, and then it turns out that some odd ioctl or
> other thing ends up doing something odd and interesting.
> 
> If Al has actual performance profiles showing that the access_ok() is
> a real problem, then fine. As a low-level optimization, I agree with
> it. But not as a "let's just drop them, and make the security rules be
> non-local and subtle, and require people to know the details of the
> whole call-chain".
> 
> Seeing a "__get_user()" and just being able to glance up in the same
> function and seeing the "access_ok()" is just a good safety net. And
> means that people don't have to waste time thinking about or looking
> for where the hell the security net really is.

Umm...  It's not quite that bad - the thing is, for iov_iter-net series
we'll need copy_and_csum_{to,from}_iter() anyway and for iovec-backed
iov_iter instances we already ask the iovec to be validated wrt access_ok().
And this validation (already done by rw_copy_check_uvector()) is going to
be next to iov_iter_init() setting the iov_iter up.

Moreover, I'm planning to take iov_iter_init() into rw_copy_check_uvector().
That way setting ->iov is done from the same function that has just checked
all ranges.  If you look at the callers, you'll see that almost all of them
are directly followed by iov_iter_init() and folding it in is a fairly
obvious cleanup.

The thing is, with ..._iter() variants added we are left with no other
in-tree callers of csum_and_copy_{to,from}_user().  I agree that dropping
those access_ok() is too early at this point - the analysis of paths
by which a range can reach them is scary right now.  Moreover, it mostly
parallels the changes later in the series - ones that propagate a pointer
to iov_iter put into msdghdr in place of ->msg_iov/->msg_iovlen down to
the new primitives.  _After_ those steps the analysis (see the horrors in
commit message of 3/5) becomes trivial.

So whether we end up removing those access_ok() or not, this is not the
time to do so.  It still might make sense in the end, but not right now.

Frankly, my preference would be to provide __csum_and_copy_...() that do
not bother with access_ok() (and do so consistently between the architectures),
and do not bother with zeroing or trying to do an accurate csum in case of
error.  With uniform implementation of csum_and_copy_...() that does
access_ok(), tries to call __csum_... variant and, in case of failure,
does __copy_..._user(), zeroes the tail in the "from" one and calculates
the csum by source of destination - whichever's kernel-side.  I.e. do what
ppc64 is doing.  That way we get obviously safe csum_and_copy_.._user(),
and consistent __ counterparts directly used by ..._iter() primitives.
Quite a bit of complexity becomes possible to remove from asm code,
while we are at it - zeroing isn't the worst of it, contortions needed to
calculate the csum accurately in error case are often nastier.  So much
that e.g. arm doesn't even bother trying.

Again, this is a separate work - I agree with you regarding the overhead
being a non-issue for existing callers, and if somebody tries e.g. to send
64K from 32K-element vector of 2-byte ranges they'll have a _lot_ of other
overhead that will drown that of those access_ok().  In normal cases the
price of copying the data itself is going to swamp that of access_ok(),
of course.

IOW, consider the access_ok() changes withdrawn for now.  It's too early
in the series for them and the only reason to pull them that high in
ordering had been the fear of overhead.  Which is very unlikely to be
an issue.  They won't come back (if they come back at all) until the
proof of correctness becomes absolutely trivial.

^ permalink raw reply

* Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Keller, Jacob E @ 2014-11-19 21:06 UTC (permalink / raw)
  To: richardcochran@gmail.com
  Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
	Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <20141119202604.GA22213@localhost.localdomain>

On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > Good catch :)
> 
> (Well, my X session suddenly disappeared, and a kernel oops appeared in
> the console... hard to overlook ;^)
>  
> > Did you see my concern about the reset path needing to fully restore the
> > state since it is called after a hardware MAC reset which has cleared
> > all these registers?
> 
> Yes, and that bit I copied from the first series a year ago. I don't
> remember why, but IIRC that was necessary to let the SDP stuff work at
> all. Maybe the reset function was called under different circumstances
> back then. I'll take another look.
> 
> I find it a bit weird that the auxiliary functions don't work when the
> interface or the link is down.
> 
> Thanks,
> Richard

I think you need something here, but it should be clearing that register
after a MAC reset, so it needs to be re-initialized. I'm not sure if
that reset path was used in the same place in the past.

Well, I think igb and ixgbe destroy the ptp device when the interface
goes down, and restore it when it comes up. Probably we should instead
handle some things in reset path and allow the ptp device to remain.

It's partly due to the clock speed changing based on link speed.

Regards,
Jake

^ permalink raw reply

* Re: BCM4313 & brcmsmac & 3.12: only semi-working?
From: Michael Tokarev @ 2014-11-19 21:00 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Maximilian Engelhardt, Rafał Miłecki, Seth Forshee,
	brcm80211 development, linux-wireless@vger.kernel.org,
	Network Development
In-Reply-To: <546CF66A.3000207@msgid.tls.msk.ru>

19.11.2014 22:58, Michael Tokarev wrote:
> 19.11.2014 20:54, Arend van Spriel wrote:
[]
>> I submitted two patches upstream and additionally I have attached two other that are still under review. Could you try these patches and sent me the content of the two debugfs files 'macstat' and 'hardware' after a stall has occurred.
> 
> You didn't tell which kernel it is based on.  So I tried it on 3.16,

Ok, I misunderstood you apparently, -- I only tried 2 patches,
while I should try all 4.  So here it goes.

The hardware info again:

> chipnum 0x4313
> chiprev 0x1
> chippackage 0x8
> corerev 0x18
> boardid 0x1795
> boardvendor 0x103c
> boardrev P107
> boardflags 0x402201
> boardflags2 0x884
> ucoderev 0x262032c
> radiorev 0x1
> phytype 0x8
> phyrev 0x1
> anarev 0xa
> nvramrev 8

Macstat:

txallfrm: 287
txrtsfrm: 118
txctsfrm: 25
txackfrm: 60
txdnlfrm: 0
txbcnfrm: 0
txfunfl[8]: 0 0 0 0 0 0 0 0
txtplunfl: 0
txphyerr: 0
pktengrxducast: 0
pktengrxdmcast: 0
rxfrmtoolong: 330
rxfrmtooshrt: 16
rxinvmachdr: 722
rxbadfcs: 4306
rxbadplcp: 7257
rxcrsglitch: 61757
rxstrt: 6667
rxdfrmucastmbss: 41
rxmfrmucastmbss: 25
rxcfrmucast: 116
rxrtsucast: 0
rxctsucast: 59
rxackucast: 19
rxdfrmocast: 70
rxmfrmocast: 84
rxcfrmocast: 211
rxrtsocast: 3
rxctsocast: 20
rxdfrmmcast: 9
rxmfrmmcast: 1486
rxcfrmmcast: 0
rxbeaconmbss: 377
rxdfrmucastobss: 0
rxbeaconobss: 1086
rxrsptmout: 94
bcntxcancl: 0
rxf0ovfl: 0
rxf1ovfl: 0
rxf2ovfl: 0
txsfovfl: 0
pmqovfl: 0
rxcgprqfrm: 0
rxcgprsqovfl: 0
txcgprsfail: 0
txcgprssuc: 0
prs_timeout: 0
rxnack: 0
frmscons: 0
txnack: 0
txglitch_nack: 38
txburst: 4
bphy_rxcrsglitch: 2
phywatchdog: 0
bphy_badplcp: 0


As far as I can see, the stats are never updated during stall,
no numbers are changing, at least while the download is waiting
for the next packet.  Sometimes wpa_supplicant does something
little, so some stats gets updated, eg, this is how it looks like
after about 2..3 minutes:

txallfrm: 420
txrtsfrm: 201
txctsfrm: 25
txackfrm: 69
txdnlfrm: 0
txbcnfrm: 0
txfunfl[8]: 0 0 0 0 0 0 0 0
txtplunfl: 0
txphyerr: 0
pktengrxducast: 0
pktengrxdmcast: 0
rxfrmtoolong: 1908
rxfrmtooshrt: 73
rxinvmachdr: 4115
rxbadfcs: 15064
rxbadplcp: 42368
rxcrsglitch: 36620
rxstrt: 26393
rxdfrmucastmbss: 48
rxmfrmucastmbss: 27
rxcfrmucast: 158
rxrtsucast: 0
rxctsucast: 92
rxackucast: 25
rxdfrmocast: 113
rxmfrmocast: 390
rxcfrmocast: 962
rxrtsocast: 38
rxctsocast: 59
rxdfrmmcast: 48
rxmfrmmcast: 7681
rxcfrmmcast: 0
rxbeaconmbss: 1505
rxdfrmucastobss: 0
rxbeaconobss: 6059
rxrsptmout: 171
bcntxcancl: 0
rxf0ovfl: 0
rxf1ovfl: 0
rxf2ovfl: 0
txsfovfl: 0
pmqovfl: 0
rxcgprqfrm: 0
rxcgprsqovfl: 0
txcgprsfail: 0
txcgprssuc: 0
prs_timeout: 0
rxnack: 0
frmscons: 0
txnack: 0
txglitch_nack: 41
txburst: 4
bphy_rxcrsglitch: 5
phywatchdog: 0
bphy_badplcp: 0


This is with 3.18-tobe kernel (current Linus git).

Dunno if this is helpful or not...

Thanks,

/mjt

^ permalink raw reply

* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
From: Joe Stringer @ 2014-11-19 20:49 UTC (permalink / raw)
  To: Pravin Shelar; +Cc: dev@openvswitch.org, netdev, LKML
In-Reply-To: <CALnjE+otirroKNXE7qbUHfoTifipp8LJ6g8yNsLz0dBjm3PUsQ@mail.gmail.com>

On Wednesday, November 19, 2014 12:33:10 Pravin Shelar wrote:
> On Wed, Nov 19, 2014 at 11:51 AM, Joe Stringer <joestringer@nicira.com> 
wrote:
> > On Wednesday, November 19, 2014 11:08:35 Pravin Shelar wrote:
> >> On Wed, Nov 19, 2014 at 9:48 AM, Joe Stringer <joestringer@nicira.com>
> > 
> > wrote:
> >> > On Wednesday, November 19, 2014 00:11:01 Pravin Shelar wrote:
> >> >> On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer
> >> >> <joestringer@nicira.com>
> >> > 
> >> > wrote:
> >> >> > On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
> >> >> >> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer
> >> >> >> <joestringer@nicira.com>
> >> >> >> 
> >> >> >> wrote:
> >> >> >> > When userspace doesn't provide a mask, OVS datapath generates a
> >> >> >> > fully unwildcarded mask for the flow. This is done by taking a
> >> >> >> > copy of the flow key, then iterating across its attributes,
> >> >> >> > setting all values to 0xff. This works for most attributes, as
> >> >> >> > the length of the netlink attribute typically matches the
> >> >> >> > length of the value. However, IPv6 labels only use the lower 20
> >> >> >> > bits of the field. This patch makes a special case to handle
> >> >> >> > this.
> >> >> >> > 
> >> >> >> > This fixes the following error seen when installing IPv6 flows
> >> >> >> > without a mask:
> >> >> >> > 
> >> >> >> > openvswitch: netlink: Invalid IPv6 flow label value
> >> >> >> > (value=ffffffff, max=fffff)
> >> >> >> 
> >> >> >> We should allow exact match mask here rather than generating
> >> >> >> wildcarded mask. So that ovs can catch invalid ipv6.label.
> >> >> > 
> >> >> > I don't quite follow, I thought this was exact-match? (The existing
> >> >> > function sets all bits to 1)
> >> >> 
> >> >> With 0xffffffff value we can exact match on all ipv6.lable bits.
> >> > 
> >> > The label field is only 20 bits. The other bits in the same word of
> >> > the IPv6 header are for version (fixed) and traffic class (handled
> >> > separately). We don't do anything with the other bits.
> >> 
> >> This is just to make sure that we do not use those field for any thing
> >> else. Masking those extra bits can hide incorrect ipv6 key extraction.
> > 
> > Oh, I see. I meant something more like:
> > 
> > ipv6_key->ipv6_label &= htonl(0xFFF00000);
> > ipv6_key->ipv6_label |= htonl(0x000FFFFF);
> > 
> > (Which would propagate the invalid bits from the flow key, but actually
> > produce an exact match).
> 
> yes, it can wildcard unused bits.

I'll send a v2.

^ permalink raw reply

* Re: [PATCH net-net 0/4] Increase the limit of tuntap queues
From: Michael S. Tsirkin @ 2014-11-19 20:44 UTC (permalink / raw)
  To: David Miller
  Cc: pagupta, linux-kernel, netdev, jasowang, dgibson, vfalico,
	edumazet, vyasevic, hkchu, wuzhy, xemul, therbert, bhutchings,
	xii, stephen, jiri, sergei.shtylyov
In-Reply-To: <20141119.151628.768548269128919029.davem@davemloft.net>

On Wed, Nov 19, 2014 at 03:16:28PM -0500, David Miller wrote:
> From: Pankaj Gupta <pagupta@redhat.com>
> Date: Tue, 18 Nov 2014 21:52:54 +0530
> 
> > - Accept maximum number of queues as sysctl param so that any user space 
> >   application like libvirt can use this value to limit number of queues. Also
> >   Administrators can specify maximum number of queues by updating this sysctl
> >   entry.
> 
> This is the only part I don't like.
> 
> Just let whoever has privileges to configure the tun device shoot
> themselves in the foot if they want to by configuring "too many"
> queues.
> 
> If the virtual entity runs itself out of resources by doing something
> stupid, it's purely their problem.

Well it will run host out of kernel, no?

-- 
MST

^ permalink raw reply

* Re: [PATCH v3] fix locking regression in ipx_sendmsg and ipx_recvmsg
From: David Miller @ 2014-11-19 20:44 UTC (permalink / raw)
  To: jbohac; +Cc: arnd, acme, netdev
In-Reply-To: <20141119103814.GB19092@midget.suse.cz>

From: Jiri Bohac <jbohac@suse.cz>
Date: Wed, 19 Nov 2014 11:38:14 +0100

> This fixes an old regression introduced by commit
> b0d0d915 (ipx: remove the BKL).
> 
> When a recvmsg syscall blocks waiting for new data, no data can be sent on the
> same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
> 
> This breaks mars-nwe (NetWare emulator):
> - the ncpserv process reads the request using recvmsg
> - ncpserv forks and spawns nwconn
> - ncpserv calls a (blocking) recvmsg and waits for new requests
> - nwconn deadlocks in sendmsg on the same socket 
> 
> Commit b0d0d915 has simply replaced BKL locking with
> lock_sock/release_sock. Unlike now, BKL got unlocked while
> sleeping, so a blocking recvmsg did not block a concurrent
> sendmsg.
> 
> Only keep the socket locked while actually working with the socket data and
> release it prior to calling skb_recv_datagram(). 
> 
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

Please fix your Subject line to have a proper subsystem prefix, in this
case "ipx: " is sufficient.

In fact, I think your previous versions has the subject line setup
correctly wrt. this, why did you break it? :-)

> @@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	struct ipxhdr *ipx = NULL;
>  	struct sk_buff *skb;
>  	int copied, rc;
> +	int locked = 1;
>  
>  	lock_sock(sk);
>  	/* put the autobinding in */

Please use 'bool' and true/false.

^ 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