Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] usbnet: Fix dma setup for fragmented packets that need a pad byte appended.
From: Eric Dumazet @ 2014-01-14 14:41 UTC (permalink / raw)
  To: David Laight; +Cc: netdev
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D45B28F@AcuExch.aculab.com>

On Tue, 2014-01-14 at 11:16 +0000, David Laight wrote:
> If the usbnet code appends a byte to a fragmented packet (in order to avoid
> sending a bulk data message that is a multiple of the USB message size) then
> the scatter-gather list isn't initialised correctly.
> This causes a later panic in usb_hcd_map_urb_for_dma().
> Basically when the code tries to access the final sg fragment the sg function
> returns NULL because the 'end of sg list' market is set in the previous one.
> 
> Bug introduced in commit 60e453a940ac678565b6641d65f8c18541bb9f28
> (USBNET: fix handling padding packet) and needs applying to all
> kernels that contain this change (including 3.12).
> 
> Fix from Bjorn Mork.
> 
> Signed-off-by: David Laight <david.laight@aculab.com>
> ---
> 
> I think it is ok that the sg table's last element is never assigned to when
> the packet isn't padded.

Original patch contained :

Fixes: 60e453a940ac ("USBNET: fix handling padding packet")
Reported-by: Thomas Kear <thomas@kear.co.nz>
Cc: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>

Why did you remove this and took ownership of this patch ?

It seems you would only need to either add :

Acked-by: ...

or 

Tested-by: ...

or

Reviewed-by: ...

^ permalink raw reply

* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get (v3)
From: Eric Dumazet @ 2014-01-14 14:36 UTC (permalink / raw)
  To: Andrew Vagin
  Cc: Florian Westphal, Andrey Vagin, netfilter-devel, netfilter,
	coreteam, netdev, linux-kernel, vvs, Pablo Neira Ayuso,
	Patrick McHardy, Jozsef Kadlecsik, David S. Miller,
	Cyrill Gorcunov
In-Reply-To: <20140114105147.GA14538@paralelels.com>

On Tue, 2014-01-14 at 14:51 +0400, Andrew Vagin wrote:

> I think __nf_conntrack_alloc must use atomic_inc instead of
> atomic_set. And we must be sure, that the first object from a new page is
> zeroized.

No you can not do that, and we do not need.

If a new page is allocated, then you have the guarantee nobody can ever
uses it, its content can be totally random.

Only 'living' objects, the ones that were previously inserted in the
hash table, can be found, and their refcnt must be accurate.

A freed object has refcnt == 0, thats the golden rule.

When the page is freed (after RCU grace period), nobody cares of refcnt
anymore.




^ permalink raw reply

* Re: [net-next 5/6] i40e: trivial cleanup
From: Sergei Shtylyov @ 2014-01-14 14:22 UTC (permalink / raw)
  To: Aaron Brown, davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann
In-Reply-To: <1389689394-22369-6-git-send-email-aaron.f.brown@intel.com>

Hello.

On 14-01-2014 12:49, Aaron Brown wrote:

> From: Jesse Brandeburg <jesse.brandeburg@intel.com>

> Remove some un-necessary parenthesis.

> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
> Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c b/drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c
> index 101ed41..d5d98fe 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c
> @@ -485,8 +485,7 @@ i40e_status i40e_configure_lan_hmc(struct i40e_hw *hw,
>   		/* Make one big object, a single SD */
>   		info.count = 1;
>   		ret_code = i40e_create_lan_hmc_object(hw, &info);
> -		if ((ret_code) &&
> -		    (model == I40E_HMC_MODEL_DIRECT_PREFERRED))
> +		if (ret_code && (model == I40E_HMC_MODEL_DIRECT_PREFERRED))

    Parens around == are also unnecessary, if you're at it.

WBR, Sergei

^ permalink raw reply

* Re: [PATCH net-next 1/2] net: vxlan: when lower dev unregisters remove vxlan dev as well
From: Daniel Borkmann @ 2014-01-14 14:02 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: davem, netdev
In-Reply-To: <20140113182232.355f9d20@nehalam.linuxnetplumber.net>

On 01/14/2014 03:22 AM, Stephen Hemminger wrote:
> On Mon, 13 Jan 2014 18:41:19 +0100
> Daniel Borkmann <dborkman@redhat.com> wrote:
>
>> We can create a vxlan device with an explicit underlying carrier.
>> In that case, when the carrier link is being deleted from the
>> system (e.g. due to module unload) we should also clean up all
>> created vxlan devices on top of it since otherwise we're in an
>> inconsistent state in vxlan device. In that case, the user needs
>> to remove all such devices, while in case of other virtual devs
>> that sit on top of physical ones, it is usually the case that
>> these devices do unregister automatically as well and do not
>> leave the burden on the user.
>>
>> This work is not necessary when vxlan device was not created with
>> a real underlying device, as connections can resume in that case
>> when driver is plugged again. But at least for the other cases,
>> we should go ahead and do the cleanup on removal.
>>
>> We don't register the notifier during vxlan_newlink() here since
>> I consider this event rather rare, and therefore we should not
>> bloat vxlan's core structure unecessary. Also, we can simply make
>> use of unregister_netdevice_many() to batch that. fdb is flushed
>> upon ndo_stop().
>>
>> E.g. `ip -d link show vxlan13` after carrier removal before
>> this patch:
>>
>> 5: vxlan13: <BROADCAST,MULTICAST> mtu 1450 qdisc noop state DOWN mode DEFAULT group default
>>      link/ether 1e:47:da:6d:4d:99 brd ff:ff:ff:ff:ff:ff promiscuity 0
>>      vxlan id 13 group 239.0.0.10 dev 2 port 32768 61000 ageing 300
>>                                   ^^^^^
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>
> Since vxlan is running over UDP socket. I wonder if this could be
> done better by implementing something equivalent to SO_BINDTODEVICE.
>
> What happens to a user land application which has a UDP socket
> and has done SO_BINDTODEVICE and device is removed? Is there an asynchronous
> error, can the application recover? Why can't vxlan use the same mechanism?

Interesting point. What seems to happen with UDP sockets and SO_BINDTODEVICE
in case the device was present during the setsockopt(2), and module was
unloaded at time of sendto(2)/recvfrom(2), that at least senders give an
error of ENODEV in such cases while receivers seem not to notice as far as
I can tell. When we reload the module and device appears again with the
same name, then sendto(2), still fails with ENODEV, since we, of course,
work with indexes, that is, sk->sk_bound_dev_if. The only chance user space
would have is to try to redo the setsockopt(2) with SO_BINDTODEVICE on the
same device _name_, but different new index now, in the hope that this would
be the same underlying NIC. It seems, however not recommended to do so [1].
Anyway, so I'm not sure how useful this would be, I guess just doing what
this patch here does should be appropriate to do.

   [1] http://ftp.riken.go.jp/Linux/kernel/v2.0/patch-html/patch-2.0.31/linux_Documentation_networking_so_bindtodevice.txt.html

^ permalink raw reply

* [PATCH][net-next] gianfar: Fix portabilty issues for ethtool and ptp
From: Claudiu Manoil @ 2014-01-14 13:35 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller

Fixes unhandled register write in gianfar_ethtool.c.
Fixes following endianess related functional issues,
reported by sparse as well, i.e.:

gianfar_ethtool.c:1058:33: warning:
    incorrect type in argument 1 (different base types)
    expected unsigned int [unsigned] [usertype] value
    got restricted __be32 [usertype] ip4src

gianfar_ethtool.c:1164:33: warning:
    restricted __be16 degrades to integer

gianfar_ethtool.c:1669:32: warning:
    invalid assignment: ^=
    left side has type restricted __be16
    right side has type int

Solves all the sparse warnings for mixig normal pointers
with __iomem pointers for gianfar_ptp.c, i.e.:
gianfar_ptp.c:163:32: warning:
    incorrect type in argument 1 (different address spaces)
    expected unsigned int [noderef] <asn:2>*addr
    got unsigned int *<noident>

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 98 +++++++++++++++++-------
 drivers/net/ethernet/freescale/gianfar_ptp.c     |  2 +-
 2 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index d3d7ede..5900dba 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -889,11 +889,9 @@ static int gfar_set_hash_opts(struct gfar_private *priv,
 
 static int gfar_check_filer_hardware(struct gfar_private *priv)
 {
-	struct gfar __iomem *regs = NULL;
+	struct gfar __iomem *regs = priv->gfargrp[0].regs;
 	u32 i;
 
-	regs = priv->gfargrp[0].regs;
-
 	/* Check if we are in FIFO mode */
 	i = gfar_read(&regs->ecntrl);
 	i &= ECNTRL_FIFM;
@@ -927,7 +925,7 @@ static int gfar_check_filer_hardware(struct gfar_private *priv)
 	/* Sets the properties for arbitrary filer rule
 	 * to the first 4 Layer 4 Bytes
 	 */
-	regs->rbifx = 0xC0C1C2C3;
+	gfar_write(&regs->rbifx, 0xC0C1C2C3);
 	return 0;
 }
 
@@ -1055,10 +1053,18 @@ static void gfar_set_basic_ip(struct ethtool_tcpip4_spec *value,
 			      struct ethtool_tcpip4_spec *mask,
 			      struct filer_table *tab)
 {
-	gfar_set_attribute(value->ip4src, mask->ip4src, RQFCR_PID_SIA, tab);
-	gfar_set_attribute(value->ip4dst, mask->ip4dst, RQFCR_PID_DIA, tab);
-	gfar_set_attribute(value->pdst, mask->pdst, RQFCR_PID_DPT, tab);
-	gfar_set_attribute(value->psrc, mask->psrc, RQFCR_PID_SPT, tab);
+	gfar_set_attribute(be32_to_cpu(value->ip4src),
+			   be32_to_cpu(mask->ip4src),
+			   RQFCR_PID_SIA, tab);
+	gfar_set_attribute(be32_to_cpu(value->ip4dst),
+			   be32_to_cpu(mask->ip4dst),
+			   RQFCR_PID_DIA, tab);
+	gfar_set_attribute(be16_to_cpu(value->pdst),
+			   be16_to_cpu(mask->pdst),
+			   RQFCR_PID_DPT, tab);
+	gfar_set_attribute(be16_to_cpu(value->psrc),
+			   be16_to_cpu(mask->psrc),
+			   RQFCR_PID_SPT, tab);
 	gfar_set_attribute(value->tos, mask->tos, RQFCR_PID_TOS, tab);
 }
 
@@ -1067,12 +1073,17 @@ static void gfar_set_user_ip(struct ethtool_usrip4_spec *value,
 			     struct ethtool_usrip4_spec *mask,
 			     struct filer_table *tab)
 {
-	gfar_set_attribute(value->ip4src, mask->ip4src, RQFCR_PID_SIA, tab);
-	gfar_set_attribute(value->ip4dst, mask->ip4dst, RQFCR_PID_DIA, tab);
+	gfar_set_attribute(be32_to_cpu(value->ip4src),
+			   be32_to_cpu(mask->ip4src),
+			   RQFCR_PID_SIA, tab);
+	gfar_set_attribute(be32_to_cpu(value->ip4dst),
+			   be32_to_cpu(mask->ip4dst),
+			   RQFCR_PID_DIA, tab);
 	gfar_set_attribute(value->tos, mask->tos, RQFCR_PID_TOS, tab);
 	gfar_set_attribute(value->proto, mask->proto, RQFCR_PID_L4P, tab);
-	gfar_set_attribute(value->l4_4_bytes, mask->l4_4_bytes, RQFCR_PID_ARB,
-			   tab);
+	gfar_set_attribute(be32_to_cpu(value->l4_4_bytes),
+			   be32_to_cpu(mask->l4_4_bytes),
+			   RQFCR_PID_ARB, tab);
 
 }
 
@@ -1139,7 +1150,41 @@ static void gfar_set_ether(struct ethhdr *value, struct ethhdr *mask,
 		}
 	}
 
-	gfar_set_attribute(value->h_proto, mask->h_proto, RQFCR_PID_ETY, tab);
+	gfar_set_attribute(be16_to_cpu(value->h_proto),
+			   be16_to_cpu(mask->h_proto),
+			   RQFCR_PID_ETY, tab);
+}
+
+static inline u32 vlan_tci_vid(struct ethtool_rx_flow_spec *rule)
+{
+	return be16_to_cpu(rule->h_ext.vlan_tci) & VLAN_VID_MASK;
+}
+
+static inline u32 vlan_tci_vidm(struct ethtool_rx_flow_spec *rule)
+{
+	return be16_to_cpu(rule->m_ext.vlan_tci) & VLAN_VID_MASK;
+}
+
+static inline u32 vlan_tci_cfi(struct ethtool_rx_flow_spec *rule)
+{
+	return be16_to_cpu(rule->h_ext.vlan_tci) & VLAN_CFI_MASK;
+}
+
+static inline u32 vlan_tci_cfim(struct ethtool_rx_flow_spec *rule)
+{
+	return be16_to_cpu(rule->m_ext.vlan_tci) & VLAN_CFI_MASK;
+}
+
+static inline u32 vlan_tci_prio(struct ethtool_rx_flow_spec *rule)
+{
+	return (be16_to_cpu(rule->h_ext.vlan_tci) & VLAN_PRIO_MASK) >>
+		VLAN_PRIO_SHIFT;
+}
+
+static inline u32 vlan_tci_priom(struct ethtool_rx_flow_spec *rule)
+{
+	return (be16_to_cpu(rule->m_ext.vlan_tci) & VLAN_PRIO_MASK) >>
+		VLAN_PRIO_SHIFT;
 }
 
 /* Convert a rule to binary filter format of gianfar */
@@ -1153,22 +1198,21 @@ static int gfar_convert_to_filer(struct ethtool_rx_flow_spec *rule,
 	u32 old_index = tab->index;
 
 	/* Check if vlan is wanted */
-	if ((rule->flow_type & FLOW_EXT) && (rule->m_ext.vlan_tci != 0xFFFF)) {
+	if ((rule->flow_type & FLOW_EXT) &&
+	    (rule->m_ext.vlan_tci != cpu_to_be16(0xFFFF))) {
 		if (!rule->m_ext.vlan_tci)
-			rule->m_ext.vlan_tci = 0xFFFF;
+			rule->m_ext.vlan_tci = cpu_to_be16(0xFFFF);
 
 		vlan = RQFPR_VLN;
 		vlan_mask = RQFPR_VLN;
 
 		/* Separate the fields */
-		id = rule->h_ext.vlan_tci & VLAN_VID_MASK;
-		id_mask = rule->m_ext.vlan_tci & VLAN_VID_MASK;
-		cfi = rule->h_ext.vlan_tci & VLAN_CFI_MASK;
-		cfi_mask = rule->m_ext.vlan_tci & VLAN_CFI_MASK;
-		prio = (rule->h_ext.vlan_tci & VLAN_PRIO_MASK) >>
-		       VLAN_PRIO_SHIFT;
-		prio_mask = (rule->m_ext.vlan_tci & VLAN_PRIO_MASK) >>
-			    VLAN_PRIO_SHIFT;
+		id = vlan_tci_vid(rule);
+		id_mask = vlan_tci_vidm(rule);
+		cfi = vlan_tci_cfi(rule);
+		cfi_mask = vlan_tci_cfim(rule);
+		prio = vlan_tci_prio(rule);
+		prio_mask = vlan_tci_priom(rule);
 
 		if (cfi == VLAN_TAG_PRESENT && cfi_mask == VLAN_TAG_PRESENT) {
 			vlan |= RQFPR_CFI;
@@ -1666,10 +1710,10 @@ static void gfar_invert_masks(struct ethtool_rx_flow_spec *flow)
 	for (i = 0; i < sizeof(flow->m_u); i++)
 		flow->m_u.hdata[i] ^= 0xFF;
 
-	flow->m_ext.vlan_etype ^= 0xFFFF;
-	flow->m_ext.vlan_tci ^= 0xFFFF;
-	flow->m_ext.data[0] ^= ~0;
-	flow->m_ext.data[1] ^= ~0;
+	flow->m_ext.vlan_etype ^= cpu_to_be16(0xFFFF);
+	flow->m_ext.vlan_tci ^= cpu_to_be16(0xFFFF);
+	flow->m_ext.data[0] ^= cpu_to_be32(~0);
+	flow->m_ext.data[1] ^= cpu_to_be32(~0);
 }
 
 static int gfar_add_cls(struct gfar_private *priv,
diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c b/drivers/net/ethernet/freescale/gianfar_ptp.c
index e006a09..6ba2fd4 100644
--- a/drivers/net/ethernet/freescale/gianfar_ptp.c
+++ b/drivers/net/ethernet/freescale/gianfar_ptp.c
@@ -134,7 +134,7 @@ struct gianfar_ptp_registers {
 #define REG_SIZE	sizeof(struct gianfar_ptp_registers)
 
 struct etsects {
-	struct gianfar_ptp_registers *regs;
+	struct gianfar_ptp_registers __iomem *regs;
 	spinlock_t lock; /* protects regs */
 	struct ptp_clock *clock;
 	struct ptp_clock_info caps;
-- 
1.7.11.7

^ permalink raw reply related

* Re: [PATCH net-next] IPv6: add option to use anycast addresses as source addresses in icmp error messages
From: Hannes Frederic Sowa @ 2014-01-14 13:13 UTC (permalink / raw)
  To: Francois-Xavier Le Bail
  Cc: netdev, Bill Fink, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki Yoshifuji, Patrick McHardy
In-Reply-To: <1389633764-5210-1-git-send-email-fx.lebail@yahoo.com>

On Mon, Jan 13, 2014 at 06:22:44PM +0100, Francois-Xavier Le Bail wrote:
> - Add "anycast_src_icmp_error" sysctl to control the use of anycast addresses
>   as source addresses for ICMPv6 error messages. This sysctl is false by
>   default to preserve existing behavior.
> - Use it in icmp6_send().
> 
> Suggested-by: Bill Fink <billfink@mindspring.com>
> Signed-off-by: Francois-Xavier Le Bail <fx.lebail@yahoo.com>

Regarding the anycast patches, I contacted someone from IETF.

The number of sysctls needed to get introduced to have all the flexibility
regarding source address selection and don't break backward compatibility
concerns me a bit.

Especially on end hosts, where those switches will be important, I think we
really have to think about sensible defaults without breaking current
software.

I currently consider a per-address flag, if those anycast addresses
should be available in source address selection (also with an enhancement to
current IPV6_JOIN_ANYCAST logic).

Greetings,

  Hannes

^ permalink raw reply

* Re: [PATCH net-next] IPv6: move the anycast_src_echo_reply sysctl to netns_sysctl_ipv6
From: Hannes Frederic Sowa @ 2014-01-14 13:06 UTC (permalink / raw)
  To: Francois-Xavier Le Bail
  Cc: netdev, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki Yoshifuji, Patrick McHardy
In-Reply-To: <1389625141-3076-1-git-send-email-fx.lebail@yahoo.com>

On Mon, Jan 13, 2014 at 03:59:01PM +0100, Francois-Xavier Le Bail wrote:
> This change move anycast_src_echo_reply sysctl with other ipv6 sysctls.
> 
> Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Francois-Xavier Le Bail <fx.lebail@yahoo.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks!

^ permalink raw reply

* Re: [PATCH RESEND net-next v2 1/3] bonding: update the primary slave when changing slave's name
From: Ding Tianhong @ 2014-01-14 12:52 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <20140114105112.GC20066@redhat.com>

On 2014/1/14 18:51, Veaceslav Falico wrote:
> On Tue, Jan 14, 2014 at 05:08:21PM +0800, Ding Tianhong wrote:
>> If the slave's name changed, and the bond params primary is exist,
>> the bond should deal with the situation in two ways:
>>
>> 1) If the slave was the primary slave yet, clean the primary slave
>>   and reselect active slave.
>> 2) If the slave's new name is as same as bond primary, set the slave
>>   as primary slave and reselect active slave.
>>
>> Thanks for Veaceslav's suggestion.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e06c445..64e25d5 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2860,9 +2860,29 @@ static int bond_slave_netdev_event(unsigned long event,
>>          */
>>         break;
>>     case NETDEV_CHANGENAME:
>> -        /*
>> -         * TODO: handle changing the primary's name
>> +        /* Handle changing the slave's name:
>> +         * 1) If the slave was primary slave,
>> +         * clean the primary slave and reselect
>> +         * active slave.
>> +         * 2) If the slave's new name is same as
>> +         * bond primary, set the slave as primary
>> +         * slave and reselect active slave.
>>          */
>> +        if (slave == bond->primary_slave ||
>> +            !strcmp(bond->params.primary, slave_dev->name)) {
> 
> And if we're in a mode that doesn't use primary, but have the
> params.primary set? Then we'll issue a bond_select_active_slave() in, say,
> 802.3ad mode.
> 
> In the past 24h I've nacked about 5 of your patchsets, with you keeping
> 'quickfixing' them, without getting your time to understand the issues, and
> re-sending them for review.
> 
> I'm not willing to waste my time that uselessly, reviewing patchsets that
> you randomly generate in the hope of getting it right. And given your
> 'good' history - with patchsets that cause regressions and bugs, with
> reverts because of that, with those horrible, meaningless RCU transition
> that is just plainly wrong and *really* hard to fix - I'm going to react as
> Greg KH said in one of his presentations - NAK your patches and make them
> by myself. It will take *a lot* lesser time from my side, and will
> eventually make the code better.
> 
> Thanks for the report, I'll send a patch that fixes it soon.
> 
> Nacked-by: Veaceslav Falico <vfalico@redhat.com>

Maybe I am not in the state all day, always miss here and miss there, sorry for that.

> 
>> +            if (bond->primary_slave) {
>> +                pr_info("%s: Setting primary slave to None.\n",
>> +                    bond->dev->name);
>> +                bond->primary_slave = NULL;
>> +            } else {
>> +                pr_info("%s: Setting %s as primary slave.\n",
>> +                    bond->dev->name, slave_dev->name);
>> +                bond->primary_slave = slave;
>> +            }
>> +            write_lock_bh(&bond->curr_slave_lock);
>> +            bond_select_active_slave(bond);
>> +            write_unlock_bh(&bond->curr_slave_lock);
>> +        }
>>         break;
>>     case NETDEV_FEAT_CHANGE:
>>         bond_compute_features(bond);
>> -- 
>> 1.8.0
>>
>>
> 
> .
> 

^ permalink raw reply

* Re: [PATCH net-next 07/10] batman-adv: use __dev_get_by_index instead of dev_get_by_index to find interface
From: Antonio Quartulli @ 2014-01-14 12:43 UTC (permalink / raw)
  To: Ying Xue, davem
  Cc: vfalico, john.r.fastabend, stephen, dmitry.tarnyagin, socketcan,
	johannes, netdev,
	The list for a Better Approach To Mobile Ad-hoc Networking
In-Reply-To: <1389685269-18600-8-git-send-email-ying.xue@windriver.com>

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

On 14/01/14 08:41, Ying Xue wrote:
> The following call chains indicate that batadv_is_on_batman_iface()
> is always under rtnl_lock protection as call_netdevice_notifier()
> is protected by rtnl_lock. So if __dev_get_by_index() rather than
> dev_get_by_index() is used to find interface handler in it, this
> would help us avoid to change interface reference counter.
> 
> call_netdevice_notifier()
>   batadv_hard_if_event()
>     batadv_hardif_add_interface()
>       batadv_is_valid_iface()
>         batadv_is_on_batman_iface()
> 
> Cc: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Ying Xue <ying.xue@windriver.com>

Acked-by: Antonio Quartulli <antonio@meshcoding.com>


> ---
>  net/batman-adv/hard-interface.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
> index bebd46c..115d14e 100644
> --- a/net/batman-adv/hard-interface.c
> +++ b/net/batman-adv/hard-interface.c
> @@ -86,15 +86,13 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
>  		return false;
>  
>  	/* recurse over the parent device */
> -	parent_dev = dev_get_by_index(&init_net, net_dev->iflink);
> +	parent_dev = __dev_get_by_index(&init_net, net_dev->iflink);
>  	/* if we got a NULL parent_dev there is something broken.. */
>  	if (WARN(!parent_dev, "Cannot find parent device"))
>  		return false;
>  
>  	ret = batadv_is_on_batman_iface(parent_dev);
>  
> -	if (parent_dev)
> -		dev_put(parent_dev);
>  	return ret;
>  }
>  
> 


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [ovs-dev] [GIT net-next] Open vSwitch
From: Zoltan Kiss @ 2014-01-14 12:31 UTC (permalink / raw)
  To: Thomas Graf, Jesse Gross; +Cc: David Miller, dev@openvswitch.org, netdev
In-Reply-To: <52D50767.6050906@redhat.com>

On 14/01/14 09:46, Thomas Graf wrote:
> On 01/14/2014 02:30 AM, Jesse Gross wrote:
>>> And it works. I guess the last one causing the problem. Might be an
>>> important factor, I'm using 32 bit Dom0.
>>
>> I think you're probably right. Thomas - can you take a look?
>>
>> We shouldn't be doing any zerocopy in this situation but it looks to
>> me like we don't do any padding at all, even in situations where we
>> are copying the data.
>
> I'm looking into this now. The zerocopy method should only be attempted
> if user space has announced the ability to received unaligned messages.
>
> @Zoltan: I assume you are using an unmodified OVS 1.9.3?
>
Well, there are a few changes, but none of them seems to be significant. 
Most of them are just making OVS work with our build system. Here is the 
patchqueue:

https://github.com/xenserver/openvswitch.pg/tree/master/master

See the series file for the order of the patches.

Zoli

^ permalink raw reply

* Re: [PATCH net-next] net: make dev_set_mtu() honor notification return code
From: Veaceslav Falico @ 2014-01-14 12:13 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, jiri, edumazet, alexander.h.duyck, nicolas.dichtel
In-Reply-To: <20140113.151855.1046745397621207165.davem@davemloft.net>

On Mon, Jan 13, 2014 at 03:18:55PM -0800, David Miller wrote:
>From: Veaceslav Falico <vfalico@redhat.com>
>Date: Fri, 10 Jan 2014 13:48:17 +0100
>
>> Currently, after changing the MTU for a device, dev_set_mtu() calls
>> NETDEV_CHANGEMTU notification, however doesn't verify it's return code -
>> which can be NOTIFY_BAD - i.e. some of the net notifier blocks refused this
>> change, and continues nevertheless.
>>
>> To fix this, verify the return code, and if it's an error - then revert the
>> MTU to the original one, and pass the error code.
>>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>
>This is really a precariously designed code path.
>
>If one of the notifiers says NOTIFY_BAD, well we've already changed
>dev->mtu, therefore what if a packet got sent during this time?
>
>Whoever the NOTIFY_BAD signaller is, obviously cannot handle an MTU
>setting which we've already set in the netdev.  So allowing it's a
>terribly idea to allow visibility of the new MTU value until we can be
>sure everyone can handle it.
>
>The problem is that we really need a transaction of some sort to fix
>this properly.  First, we'd need to ask all the notifiers if they
>can handle the new MTU, then we somehow atomically set netdev->mtu
>and have the notifiers commit their own state changes.

Yeah, but I can't think of a method to atomically set it for both netdev
and its notifiers... As in, for example, bridge (but not only) takes the
lowest MTU of its ports.

>
>Then we'll have the stick issue of what to do if a notifier is
>unregistered between the check and the commit. :-)

Maybe you've meant 'registered between ...' ? :-) Anyway, I guess
dev_set_mtu() should always be called under RTNL, and this way we won't
have these problems. Though I might be wrong, everyone seems playing with
MTU the way they want.

>
>Your patch is an improvement so I will apply it, this stuff really
>is full of holes still.

One (least intrusive) approach would be to add NETDEV_PRECHANGEMTU, which
would be used to verify if the notifiers all agree with changing, and leave
the NETDEV_CHANGEMTU fail only when something really bad happened. That's
your idea, basically.

As, currently, only team can signal NOTIFY_BAD on mtu change, it's really
easy to implement. What do you think?

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 736050d..4e50e04 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2850,7 +2850,7 @@ static int team_device_event(struct notifier_block *unused,
  	case NETDEV_FEAT_CHANGE:
  		team_compute_features(port->team);
  		break;
-	case NETDEV_CHANGEMTU:
+	case NETDEV_PRECHANGEMTU:
  		/* Forbid to change mtu of underlaying device */
  		return NOTIFY_BAD;
  	case NETDEV_PRE_TYPE_CHANGE:
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a2a70cc..7e023c4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1731,6 +1731,7 @@ struct pcpu_sw_netstats {
  #define NETDEV_JOIN		0x0014
  #define NETDEV_CHANGEUPPER	0x0015
  #define NETDEV_RESEND_IGMP	0x0016
+#define NETDEV_PRECHANGEMTU	0x0017
  
  int register_netdevice_notifier(struct notifier_block *nb);
  int unregister_netdevice_notifier(struct notifier_block *nb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 87312dc..096d4dd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5332,6 +5332,10 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
  	if (!netif_device_present(dev))
  		return -ENODEV;
  
+	err = call_netdevice_notifiers(NETDEV_PRECHANGEMTU, dev);
+	if (!err)
+		return notifier_to_errno(err);
+
  	orig_mtu = dev->mtu;
  	err = __dev_set_mtu(dev, new_mtu);
  

>
>Thanks.
>

^ permalink raw reply related

* Re: [PATCH net-next 10/10] net: nl80211: __dev_get_by_index instead of dev_get_by_index to find interface
From: Johannes Berg @ 2014-01-14 12:11 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, vfalico, john.r.fastabend, stephen, antonio,
	dmitry.tarnyagin, socketcan, netdev, linux-kernel
In-Reply-To: <1389685269-18600-11-git-send-email-ying.xue@windriver.com>

On Tue, 2014-01-14 at 15:41 +0800, Ying Xue wrote:

> @@ -2218,10 +2194,6 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>  			rdev->wiphy.coverage_class = old_coverage_class;
>  		}
>  	}
> -
> - bad_res:
> -	if (netdev)
> -		dev_put(netdev);
>  	return result;

I believe that could be "return 0;" now, which would make the code
easier to read.

johannes

^ permalink raw reply

* Re: [PATCH net-next 02/10] bonding: use __dev_get_by_name instead of dev_get_by_name to find interface
From: Veaceslav Falico @ 2014-01-14 11:55 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, john.r.fastabend, stephen, antonio, dmitry.tarnyagin,
	socketcan, johannes, netdev, linux-kernel
In-Reply-To: <1389685269-18600-3-git-send-email-ying.xue@windriver.com>

On Tue, Jan 14, 2014 at 03:41:01PM +0800, Ying Xue wrote:
>The following call chain indicates that bond_do_ioctl() is protected
>under rtnl_lock. If we use __dev_get_by_name() instead of
>dev_get_by_name() to find interface handler in it, this would
>help us avoid to change reference counter of interface once.
>
>dev_ioctl()
>  rtnl_lock()
>  dev_ifsioc()
>    bond_do_ioctl()
>  rtnl_unlock()
>
>Additionally we also change the coding style in bond_do_ioctl(),
>letting it more readable for us.
>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>Signed-off-by: Ying Xue <ying.xue@windriver.com>
>---
> drivers/net/bonding/bond_main.c |   49 ++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e06c445..a69afbf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3213,37 +3213,34 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
> 	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> 		return -EPERM;
>
>-	slave_dev = dev_get_by_name(net, ifr->ifr_slave);
>+	slave_dev = __dev_get_by_name(net, ifr->ifr_slave);
>
> 	pr_debug("slave_dev=%p:\n", slave_dev);
>
> 	if (!slave_dev)
>-		res = -ENODEV;
>-	else {
>-		pr_debug("slave_dev->name=%s:\n", slave_dev->name);
>-		switch (cmd) {
>-		case BOND_ENSLAVE_OLD:
>-		case SIOCBONDENSLAVE:
>-			res = bond_enslave(bond_dev, slave_dev);
>-			break;
>-		case BOND_RELEASE_OLD:
>-		case SIOCBONDRELEASE:
>-			res = bond_release(bond_dev, slave_dev);
>-			break;
>-		case BOND_SETHWADDR_OLD:
>-		case SIOCBONDSETHWADDR:
>-			bond_set_dev_addr(bond_dev, slave_dev);
>-			res = 0;
>-			break;
>-		case BOND_CHANGE_ACTIVE_OLD:
>-		case SIOCBONDCHANGEACTIVE:
>-			res = bond_option_active_slave_set(bond, slave_dev);
>-			break;
>-		default:
>-			res = -EOPNOTSUPP;
>-		}
>+		return -ENODEV;
>
>-		dev_put(slave_dev);
>+	pr_debug("slave_dev->name=%s:\n", slave_dev->name);
>+	switch (cmd) {
>+	case BOND_ENSLAVE_OLD:
>+	case SIOCBONDENSLAVE:
>+		res = bond_enslave(bond_dev, slave_dev);
>+		break;
>+	case BOND_RELEASE_OLD:
>+	case SIOCBONDRELEASE:
>+		res = bond_release(bond_dev, slave_dev);
>+		break;
>+	case BOND_SETHWADDR_OLD:
>+	case SIOCBONDSETHWADDR:
>+		bond_set_dev_addr(bond_dev, slave_dev);
>+		res = 0;
>+		break;
>+	case BOND_CHANGE_ACTIVE_OLD:
>+	case SIOCBONDCHANGEACTIVE:
>+		res = bond_option_active_slave_set(bond, slave_dev);
>+		break;
>+	default:
>+		res = -EOPNOTSUPP;
> 	}
>
> 	return res;
>-- 
>1.7.9.5
>

^ permalink raw reply

* [PATCH v2 net-next] bonding: handle slave's name change with primary_slave logic
From: Veaceslav Falico @ 2014-01-14 11:49 UTC (permalink / raw)
  To: netdev; +Cc: Veaceslav Falico, Ding Tianhong, Jay Vosburgh, Andy Gospodarek

Currently, if a slave's name change, we just pass it by. However, if the
slave is a current primary_slave, then we end up with using a slave, whose
name != params.primary, for primary_slave. And vice-versa, if we don't have
a primary_slave but have params.primary set - we will not detected a new
primary_slave.

Fix this by catching the NETDEV_CHANGENAME event and setting primary_slave
accordingly. Also, if the primary_slave was changed, issue a reselection of
the active slave, cause the priorities have changed.

Reported-by: Ding Tianhong <dingtianhong@huawei.com>
CC: Ding Tianhong <dingtianhong@huawei.com>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..8077199 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2860,9 +2860,26 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		break;
 	case NETDEV_CHANGENAME:
-		/*
-		 * TODO: handle changing the primary's name
-		 */
+		/* we don't care if we don't have primary set */
+		if (!USES_PRIMARY(bond->params.mode) ||
+		    !bond->params.primary[0])
+			break;
+
+		if (slave == bond->primary_slave) {
+			/* slave's name changed - he's no longer primary */
+			bond->primary_slave = NULL;
+		} else if (!strcmp(slave_dev->name, bond->params.primary)) {
+			/* we have a new primary slave */
+			bond->primary_slave = slave;
+		} else  /* we didn't change primary - exit */
+			break;
+
+		pr_info("%s: Primary slave changed to %s, re-electing.\n",
+			bond->dev->name, bond->primary_slave ? slave_dev->name :
+							       "none");
+		write_lock_bh(&bond->curr_slave_lock);
+		bond_select_active_slave(bond);
+		write_unlock_bh(&bond->curr_slave_lock);
 		break;
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);
-- 
1.8.4

^ permalink raw reply related

* Re: [PATCH net-next] bonding: handle slave's name change with primary_slave logic
From: Veaceslav Falico @ 2014-01-14 11:50 UTC (permalink / raw)
  To: netdev; +Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389696645-9101-1-git-send-email-vfalico@redhat.com>

On Tue, Jan 14, 2014 at 11:50:45AM +0100, Veaceslav Falico wrote:
>Currently, if a slave's name change, we just pass it by. However, if the
>slave is a current primary_slave, then we end up with using a slave, whose
>name != params.primary, for primary_slave. And vice-versa, if we don't have
>a primary_slave but have params.primary set - we will not detected a new
>primary_slave.
>
>Fix this by catching the NETDEV_CHANGENAME event and setting primary_slave
>accordingly. Also, if the primary_slave was changed, issue a reselection of
>the active slave, cause the priorities have changed.
>
>Reported-by: Ding Tianhong <dingtianhong@huawei.com>
>CC: Ding Tianhong <dingtianhong@huawei.com>
>CC: Jay Vosburgh <fubar@us.ibm.com>
>CC: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

Self-NAK, too busy reviewing other patches.

Sent v2.

>---
> drivers/net/bonding/bond_main.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e06c445..b16d7ec 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2860,9 +2860,26 @@ static int bond_slave_netdev_event(unsigned long event,
> 		 */
> 		break;
> 	case NETDEV_CHANGENAME:
>-		/*
>-		 * TODO: handle changing the primary's name
>-		 */
>+		/* we don't care if we don't have primary set */
>+		if (!USES_PRIMARY(bond->params.mode) ||
>+		    !bond->params.primary[0])
>+			break;
>+
>+		if (slave == bond->primary_slave) {
>+			/* slave's name changed - he's no longer primary */
>+			bond->primary_slave = NULL;
>+		} else if (!strcmp(slave_dev->name, bond->params.primary)( {
>+			/* we have a new primary slave */
>+			bond->primary_slave = slave;
>+		} else  /* we didn't change primary - exit */
>+			break;
>+
>+		pr_info("%s: Primary slave changed to %s, re-electing.\n",
>+			bond->dev->name, bond->primary_slave ? slave_dev->name :
>+							       "none");
>+		write_lock_bh(&bond->curr_slave_lock);
>+		bond_select_active_slave(bond);
>+		write_unlock_bh(&bond->curr_slave_lock);
> 		break;
> 	case NETDEV_FEAT_CHANGE:
> 		bond_compute_features(bond);
>-- 
>1.8.4
>

^ permalink raw reply

* Re: [PATCH] sh_eth: fix garbled TX error message
From: Sergei Shtylyov @ 2014-01-14 11:49 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-sh
In-Reply-To: <20140113.232935.776174177146650322.davem@davemloft.net>

Hello.

On 14-01-2014 11:29, David Miller wrote:

>> sh_eth_error() in case of a TX error tries to print a message using 2 dev_err()
>> calls with the first string not finished by '\n', so that the resulting message
>> would inevitably come out garbled, with something like "3net eth0: " inserted
>> in the middle.  Avoid that by merging 2 calls into one.

>> While at it, insert an empty line after the nearby declaration.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

> Applied, thanks.

> I don't think this is really -stable material, sorry.

    Right, this is not even a fix anymore, at least not from 2.6.31 times (if 
Joe's estimate was correct), and I was going to resubmit it as a cleanup. 
Should have warned you not to apply yet but didn't, sorry.

WBR, Sergei


^ permalink raw reply

* Re: [PATCH] netfilter: Add dependency on IPV6 for NF_TABLES_INET
From: Patrick McHardy @ 2014-01-14 11:35 UTC (permalink / raw)
  To: Paul Gortmaker, David S. Miller, Pablo Neira Ayuso,
	Jozsef Kadlecsik
  Cc: netfilter, netdev
In-Reply-To: <1389636070-26312-1-git-send-email-paul.gortmaker@windriver.com>

Paul Gortmaker <paul.gortmaker@windriver.com> schrieb:
>Commit 1d49144c0aaa61be4e3ccbef9cc5c40b0ec5f2fe ("netfilter: nf_tables:
>add "inet" table for IPv4/IPv6") allows creation of non-IPV6 enabled
>.config files that will fail to configure/link as follows:
>
>warning: (NF_TABLES_INET) selects NF_TABLES_IPV6 which has unmet direct
>dependencies (NET && INET && IPV6 && NETFILTER && NF_TABLES)
>warning: (NF_TABLES_INET) selects NF_TABLES_IPV6 which has unmet direct
>dependencies (NET && INET && IPV6 && NETFILTER && NF_TABLES)
>warning: (NF_TABLES_INET) selects NF_TABLES_IPV6 which has unmet direct
>dependencies (NET && INET && IPV6 && NETFILTER && NF_TABLES)
>net/built-in.o: In function `nft_reject_eval':
>nft_reject.c:(.text+0x651e8): undefined reference to `nf_ip6_checksum'
>nft_reject.c:(.text+0x65270): undefined reference to `ip6_route_output'
>nft_reject.c:(.text+0x656c4): undefined reference to `ip6_dst_hoplimit'
>make: *** [vmlinux] Error 1
>
>Since the feature is to allow for a mixed IPV4 and IPV6 table, it
>seems sensible to make it depend on IPV6.

Acked-by: Patrick McHardy <kaber@trash.net>

>
>Cc: Patrick McHardy <kaber@trash.net>
>Cc: Pablo Neira Ayuso <pablo@netfilter.org>
>Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
>diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
>index afe50c0f526f..c37467562fd0 100644
>--- a/net/netfilter/Kconfig
>+++ b/net/netfilter/Kconfig
>@@ -429,7 +429,7 @@ config NF_TABLES
> 	  To compile it as a module, choose M here.
> 
> config NF_TABLES_INET
>-	depends on NF_TABLES
>+	depends on NF_TABLES && IPV6
> 	select NF_TABLES_IPV4
> 	select NF_TABLES_IPV6
> 	tristate "Netfilter nf_tables mixed IPv4/IPv6 tables support"

^ permalink raw reply

* Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()
From: Paul Bolle @ 2014-01-14 11:23 UTC (permalink / raw)
  To: Jack Morgenstein
  Cc: Or Gerlitz, Rony Efraim, Hadar Hen Zion, David S. Miller, netdev,
	linux-kernel
In-Reply-To: <20140114084752.1db64b21@jpm-OptiPlex-GX620>

On Tue, 2014-01-14 at 08:47 +0200, Jack Morgenstein wrote:
> On Tue, 07 Jan 2014 14:01:18 +0100
> Paul Bolle <pebolle@tiscali.nl> wrote:
> 
> > +	} else {
> > +		/* state == RES_CQ_HW */
> > +		if (r->com.state != RES_CQ_ALLOCATED)
> 
> if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
> to protect against any bad calls to this function
> (although I know that currently there are none).

So we end up with
         } else if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED) {
                 err = -EINVAL;
         } else {
                 err = 0;
         }

don't we? Which is fine with me, as GCC still is then able to correctly
analyze this function.

> This also preserves the behavior of the switch statement.
> 
> >  			err = -EINVAL;
> > -		}
> > +		else
> > +			err = 0;
> > +	}
> >  
> > -		if (!err) {
> > -			r->com.from_state = r->com.state;
> > -			r->com.to_state = state;
> > -			r->com.state = RES_CQ_BUSY;
> > -			if (cq)
> > -				*cq = r;
> > -		}
> > +	if (!err) {
> > +		r->com.from_state = r->com.state;
> > +		r->com.to_state = state;
> > +		r->com.state = RES_CQ_BUSY;
> 
> Please keep the "if" here.  Protects against (future) bad calls.
> 
> > +		*cq = r;
> >  	}

There seems to be a school of thought that says it's better to trigger
an Oops if a programming error is made (in this case by passing a NULL
cq) then silently handle that (future) programming error and make
debugging harder. But, even it that school of thought really exists,
this is up to you. Besides, it's only a triviality I added to my
patches.

Thanks for the review! I hope to send in a v2 of my patches shortly.


Paul Bolle

^ permalink raw reply

* [PATCH] usbnet: Fix dma setup for fragmented packets that need a pad byte appended.
From: David Laight @ 2014-01-14 11:16 UTC (permalink / raw)
  To: netdev

If the usbnet code appends a byte to a fragmented packet (in order to avoid
sending a bulk data message that is a multiple of the USB message size) then
the scatter-gather list isn't initialised correctly.
This causes a later panic in usb_hcd_map_urb_for_dma().
Basically when the code tries to access the final sg fragment the sg function
returns NULL because the 'end of sg list' market is set in the previous one.

Bug introduced in commit 60e453a940ac678565b6641d65f8c18541bb9f28
(USBNET: fix handling padding packet) and needs applying to all
kernels that contain this change (including 3.12).

Fix from Bjorn Mork.

Signed-off-by: David Laight <david.laight@aculab.com>
---

I think it is ok that the sg table's last element is never assigned to when
the packet isn't padded.

 drivers/net/usb/usbnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 8494bb5..aba04f5 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1245,7 +1245,7 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
 		return -ENOMEM;
 
 	urb->num_sgs = num_sgs;
-	sg_init_table(urb->sg, urb->num_sgs);
+	sg_init_table(urb->sg, urb->num_sgs + 1);
 
 	sg_set_buf(&urb->sg[s++], skb->data, skb_headlen(skb));
 	total_len += skb_headlen(skb);
-- 
1.8.1.2

^ permalink raw reply related

* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get (v3)
From: Andrey Wagin @ 2014-01-14 11:10 UTC (permalink / raw)
  To: Andrew Vagin
  Cc: Eric Dumazet, Florian Westphal, netfilter-devel, netfilter,
	coreteam, netdev, LKML, vvs, Pablo Neira Ayuso, Patrick McHardy,
	Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <20140114105147.GA14538@paralelels.com>

>
> Eh, looks like this path is incomplete too:(
>
> I think we can't set a reference counter for objects which is allocated
> from a SLAB_DESTROY_BY_RCU cache. Look at the following backtrace.
>
> cpu1                                    cpu2
> ct = ____nf_conntrack_find()
>                                         destroy_conntrack
> atomic_inc_not_zero(ct)

ct->ct_general.use is zero after destroy_conntrack(). Sorry for the noise.

^ permalink raw reply

* Re: [PATCH net-next v2.1 0/3] bonding: fix primary problem for bonding
From: Veaceslav Falico @ 2014-01-14 10:55 UTC (permalink / raw)
  To: Ding Tianhong; +Cc: Jay Vosburgh, Netdev, David S. Miller
In-Reply-To: <52D5174D.4070807@huawei.com>

On Tue, Jan 14, 2014 at 06:54:05PM +0800, Ding Tianhong wrote:
>If the slave's name changed, and the bond params primary is exist,
>the bond should deal with the situation in two ways:
>
>1) If the slave was the primary slave yet, clean the primary slave
>   and reselect active slave.
>2) If the slave's new name is as same as bond primary, set the slave
>   as primary slave and reselect active slave.
>
>If the new primary is not matching any slave in the bond, the bond should
>record it to params, clean the primary slave and select a new active slave.
>
>Update bonding.txt for primary description.
>
>v2.1 Because there are too many indentions and useless verification, so rewrite
>     the logic for updating the primary slave.
>     Modify some comments for to clean the typos.

LOL. That's exactly what I was talking about in my previous email. A quick
fix that doesn't even address the issues.

Nacked-by: Veaceslav Falico <vfalico@redhat.com>

>
>Ding Tianhong (3):
>  bonding: update the primary slave when changing slave's name
>  bonding: clean the primary slave if there is no slave matching new
>    primary
>  bonding: update bonding.txt for primary description.
>
> Documentation/networking/bonding.txt |  3 ++-
> drivers/net/bonding/bond_main.c      | 24 ++++++++++++++++++++++--
> drivers/net/bonding/bond_options.c   |  6 ++++++
> 3 files changed, 30 insertions(+), 3 deletions(-)
>
>-- 
>1.8.0
>
>
>

^ permalink raw reply

* [PATCH net-next v2.1 2/3] bonding: clean the primary slave if there is no slave matching new primary
From: Ding Tianhong @ 2014-01-14 10:54 UTC (permalink / raw)
  To: Jay Vosburgh, Veaceslav Falico, David S. Miller, Netdev

If the new primay is not matching any slave in the bond, the bond should
record it to params, clean the primary slave and select a new active slave.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_options.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 945a666..0ee0bfe 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -512,6 +512,12 @@ int bond_option_primary_set(struct bonding *bond, const char *primary)
 		}
 	}
 
+	if (bond->primary_slave) {
+		pr_info("%s: Setting primary slave to None.\n",
+			bond->dev->name);
+		bond->primary_slave = NULL;
+		bond_select_active_slave(bond);
+	}
 	strncpy(bond->params.primary, primary, IFNAMSIZ);
 	bond->params.primary[IFNAMSIZ - 1] = 0;
 
-- 
1.8.0

^ permalink raw reply related

* [PATCH net-next] bonding: handle slave's name change with primary_slave logic
From: Veaceslav Falico @ 2014-01-14 10:50 UTC (permalink / raw)
  To: netdev; +Cc: Veaceslav Falico, Ding Tianhong, Jay Vosburgh, Andy Gospodarek

Currently, if a slave's name change, we just pass it by. However, if the
slave is a current primary_slave, then we end up with using a slave, whose
name != params.primary, for primary_slave. And vice-versa, if we don't have
a primary_slave but have params.primary set - we will not detected a new
primary_slave.

Fix this by catching the NETDEV_CHANGENAME event and setting primary_slave
accordingly. Also, if the primary_slave was changed, issue a reselection of
the active slave, cause the priorities have changed.

Reported-by: Ding Tianhong <dingtianhong@huawei.com>
CC: Ding Tianhong <dingtianhong@huawei.com>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..b16d7ec 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2860,9 +2860,26 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		break;
 	case NETDEV_CHANGENAME:
-		/*
-		 * TODO: handle changing the primary's name
-		 */
+		/* we don't care if we don't have primary set */
+		if (!USES_PRIMARY(bond->params.mode) ||
+		    !bond->params.primary[0])
+			break;
+
+		if (slave == bond->primary_slave) {
+			/* slave's name changed - he's no longer primary */
+			bond->primary_slave = NULL;
+		} else if (!strcmp(slave_dev->name, bond->params.primary)( {
+			/* we have a new primary slave */
+			bond->primary_slave = slave;
+		} else  /* we didn't change primary - exit */
+			break;
+
+		pr_info("%s: Primary slave changed to %s, re-electing.\n",
+			bond->dev->name, bond->primary_slave ? slave_dev->name :
+							       "none");
+		write_lock_bh(&bond->curr_slave_lock);
+		bond_select_active_slave(bond);
+		write_unlock_bh(&bond->curr_slave_lock);
 		break;
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);
-- 
1.8.4

^ permalink raw reply related

* [PATCH net-next v2.1 0/3] bonding: fix primary problem for bonding
From: Ding Tianhong @ 2014-01-14 10:54 UTC (permalink / raw)
  To: Jay Vosburgh, Veaceslav Falico, Netdev, David S. Miller

If the slave's name changed, and the bond params primary is exist,
the bond should deal with the situation in two ways:

1) If the slave was the primary slave yet, clean the primary slave
   and reselect active slave.
2) If the slave's new name is as same as bond primary, set the slave
   as primary slave and reselect active slave.

If the new primary is not matching any slave in the bond, the bond should
record it to params, clean the primary slave and select a new active slave.

Update bonding.txt for primary description.

v2.1 Because there are too many indentions and useless verification, so rewrite
     the logic for updating the primary slave.
     Modify some comments for to clean the typos.

Ding Tianhong (3):
  bonding: update the primary slave when changing slave's name
  bonding: clean the primary slave if there is no slave matching new
    primary
  bonding: update bonding.txt for primary description.

 Documentation/networking/bonding.txt |  3 ++-
 drivers/net/bonding/bond_main.c      | 24 ++++++++++++++++++++++--
 drivers/net/bonding/bond_options.c   |  6 ++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

-- 
1.8.0

^ permalink raw reply

* [PATCH net-next v2.1 3/3] bonding: update bonding.txt for primary description
From: Ding Tianhong @ 2014-01-14 10:54 UTC (permalink / raw)
  To: Jay Vosburgh, Veaceslav Falico, David S. Miller, Netdev

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 Documentation/networking/bonding.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index a4d925e..5cdb229 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -657,7 +657,8 @@ primary
 	one slave is preferred over another, e.g., when one slave has
 	higher throughput than another.
 
-	The primary option is only valid for active-backup mode.
+	The primary option is only valid for active-backup(1),
+	balance-tlb (5) and balance-alb (6) mode.
 
 primary_reselect
 
-- 
1.8.0

^ permalink raw reply related


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