* [PATCH RESEND net-next 04/16] sfc: Move details of a Falcon bug workaround out of ethtool.c
From: Ben Hutchings @ 2013-08-22 19:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377199326.1703.49.camel@bwh-desktop.uk.level5networks.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/ethtool.c | 23 ++++-------------------
drivers/net/ethernet/sfc/falcon.c | 26 +++++++++++++++++++++++++-
drivers/net/ethernet/sfc/net_driver.h | 2 ++
drivers/net/ethernet/sfc/nic.h | 1 -
drivers/net/ethernet/sfc/workarounds.h | 2 --
5 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 1fc2145..4db37f7 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -709,7 +709,6 @@ static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
struct efx_nic *efx = netdev_priv(net_dev);
u8 wanted_fc, old_fc;
u32 old_adv;
- bool reset;
int rc = 0;
mutex_lock(&efx->mac_lock);
@@ -732,24 +731,10 @@ static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
goto out;
}
- /* TX flow control may automatically turn itself off if the
- * link partner (intermittently) stops responding to pause
- * frames. There isn't any indication that this has happened,
- * so the best we do is leave it up to the user to spot this
- * and fix it be cycling transmit flow control on this end. */
- reset = (wanted_fc & EFX_FC_TX) && !(efx->wanted_fc & EFX_FC_TX);
- if (EFX_WORKAROUND_11482(efx) && reset) {
- if (efx_nic_rev(efx) == EFX_REV_FALCON_B0) {
- /* Recover by resetting the EM block */
- falcon_stop_nic_stats(efx);
- falcon_drain_tx_fifo(efx);
- falcon_reconfigure_xmac(efx);
- falcon_start_nic_stats(efx);
- } else {
- /* Schedule a reset to recover */
- efx_schedule_reset(efx, RESET_TYPE_INVISIBLE);
- }
- }
+ /* Hook for Falcon bug 11482 workaround */
+ if (efx->type->prepare_enable_fc_tx &&
+ (wanted_fc & EFX_FC_TX) && !(efx->wanted_fc & EFX_FC_TX))
+ efx->type->prepare_enable_fc_tx(efx);
old_adv = efx->link_advertising;
old_fc = efx->wanted_fc;
diff --git a/drivers/net/ethernet/sfc/falcon.c b/drivers/net/ethernet/sfc/falcon.c
index 71998e7..6b6ac6d 100644
--- a/drivers/net/ethernet/sfc/falcon.c
+++ b/drivers/net/ethernet/sfc/falcon.c
@@ -497,7 +497,7 @@ static void falcon_reset_macs(struct efx_nic *efx)
falcon_setup_xaui(efx);
}
-void falcon_drain_tx_fifo(struct efx_nic *efx)
+static void falcon_drain_tx_fifo(struct efx_nic *efx)
{
efx_oword_t reg;
@@ -678,6 +678,28 @@ static int falcon_reconfigure_port(struct efx_nic *efx)
return 0;
}
+/* TX flow control may automatically turn itself off if the link
+ * partner (intermittently) stops responding to pause frames. There
+ * isn't any indication that this has happened, so the best we do is
+ * leave it up to the user to spot this and fix it by cycling transmit
+ * flow control on this end.
+ */
+
+static void falcon_a1_prepare_enable_fc_tx(struct efx_nic *efx)
+{
+ /* Schedule a reset to recover */
+ efx_schedule_reset(efx, RESET_TYPE_INVISIBLE);
+}
+
+static void falcon_b0_prepare_enable_fc_tx(struct efx_nic *efx)
+{
+ /* Recover by resetting the EM block */
+ falcon_stop_nic_stats(efx);
+ falcon_drain_tx_fifo(efx);
+ falcon_reconfigure_xmac(efx);
+ falcon_start_nic_stats(efx);
+}
+
/**************************************************************************
*
* PHY access via GMII
@@ -1798,6 +1820,7 @@ const struct efx_nic_type falcon_a1_nic_type = {
.set_id_led = falcon_set_id_led,
.push_irq_moderation = falcon_push_irq_moderation,
.reconfigure_port = falcon_reconfigure_port,
+ .prepare_enable_fc_tx = falcon_a1_prepare_enable_fc_tx,
.reconfigure_mac = falcon_reconfigure_xmac,
.check_mac_fault = falcon_xmac_check_fault,
.get_wol = falcon_get_wol,
@@ -1842,6 +1865,7 @@ const struct efx_nic_type falcon_b0_nic_type = {
.set_id_led = falcon_set_id_led,
.push_irq_moderation = falcon_push_irq_moderation,
.reconfigure_port = falcon_reconfigure_port,
+ .prepare_enable_fc_tx = falcon_b0_prepare_enable_fc_tx,
.reconfigure_mac = falcon_reconfigure_xmac,
.check_mac_fault = falcon_xmac_check_fault,
.get_wol = falcon_get_wol,
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index f4c7e6b..bdded38 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -946,6 +946,7 @@ static inline unsigned int efx_port_num(struct efx_nic *efx)
* @set_id_led: Set state of identifying LED or revert to automatic function
* @push_irq_moderation: Apply interrupt moderation value
* @reconfigure_port: Push loopback/power/txdis changes to the MAC and PHY
+ * @prepare_enable_fc_tx: Prepare MAC to enable pause frame TX (may be %NULL)
* @reconfigure_mac: Push MAC address, MTU, flow control and filter settings
* to the hardware. Serialised by the mac_lock.
* @check_mac_fault: Check MAC fault state. True if fault present.
@@ -995,6 +996,7 @@ struct efx_nic_type {
void (*set_id_led)(struct efx_nic *efx, enum efx_led_mode mode);
void (*push_irq_moderation)(struct efx_channel *channel);
int (*reconfigure_port)(struct efx_nic *efx);
+ void (*prepare_enable_fc_tx)(struct efx_nic *efx);
int (*reconfigure_mac)(struct efx_nic *efx);
bool (*check_mac_fault)(struct efx_nic *efx);
void (*get_wol)(struct efx_nic *efx, struct ethtool_wolinfo *wol);
diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h
index d63c299..a784363 100644
--- a/drivers/net/ethernet/sfc/nic.h
+++ b/drivers/net/ethernet/sfc/nic.h
@@ -298,7 +298,6 @@ extern void efx_nic_eventq_read_ack(struct efx_channel *channel);
extern bool efx_nic_event_present(struct efx_channel *channel);
/* MAC/PHY */
-extern void falcon_drain_tx_fifo(struct efx_nic *efx);
extern void falcon_reconfigure_mac_wrapper(struct efx_nic *efx);
extern bool falcon_xmac_check_fault(struct efx_nic *efx);
extern int falcon_reconfigure_xmac(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/workarounds.h b/drivers/net/ethernet/sfc/workarounds.h
index e4dd3a7..dff565a 100644
--- a/drivers/net/ethernet/sfc/workarounds.h
+++ b/drivers/net/ethernet/sfc/workarounds.h
@@ -30,8 +30,6 @@
/* TX_EV_PKT_ERR can be caused by a dangling TX descriptor
* or a PCIe error (bug 11028) */
#define EFX_WORKAROUND_10727 EFX_WORKAROUND_ALWAYS
-/* Transmit flow control may get disabled */
-#define EFX_WORKAROUND_11482 EFX_WORKAROUND_FALCON_AB
/* Truncated IPv4 packets can confuse the TX packet parser */
#define EFX_WORKAROUND_15592 EFX_WORKAROUND_FALCON_AB
/* Legacy ISR read can return zero once */
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH RESEND net-next 03/16] sfc: Use efx_mcdi_mon() to find efx_mcdi_mon structure from efx_nic
From: Ben Hutchings @ 2013-08-22 19:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377199326.1703.49.camel@bwh-desktop.uk.level5networks.com>
This needs to be done before we separate MCDI from siena_nic_data.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/mcdi_mon.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/sfc/mcdi_mon.c b/drivers/net/ethernet/sfc/mcdi_mon.c
index 1d552f0..fe42c3b 100644
--- a/drivers/net/ethernet/sfc/mcdi_mon.c
+++ b/drivers/net/ethernet/sfc/mcdi_mon.c
@@ -400,8 +400,7 @@ fail:
void efx_mcdi_mon_remove(struct efx_nic *efx)
{
- struct siena_nic_data *nic_data = efx->nic_data;
- struct efx_mcdi_mon *hwmon = &nic_data->hwmon;
+ struct efx_mcdi_mon *hwmon = efx_mcdi_mon(efx);
unsigned int i;
for (i = 0; i < hwmon->n_attrs; i++)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH RESEND net-next 02/16] sfc: const-qualify source pointers for MMIO write functions
From: Ben Hutchings @ 2013-08-22 19:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377199326.1703.49.camel@bwh-desktop.uk.level5networks.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/io.h | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/sfc/io.h b/drivers/net/ethernet/sfc/io.h
index 96759aee..39f6098 100644
--- a/drivers/net/ethernet/sfc/io.h
+++ b/drivers/net/ethernet/sfc/io.h
@@ -83,7 +83,7 @@ static inline __le32 _efx_readd(struct efx_nic *efx, unsigned int reg)
}
/* Write a normal 128-bit CSR, locking as appropriate. */
-static inline void efx_writeo(struct efx_nic *efx, efx_oword_t *value,
+static inline void efx_writeo(struct efx_nic *efx, const efx_oword_t *value,
unsigned int reg)
{
unsigned long flags __attribute__ ((unused));
@@ -108,7 +108,7 @@ static inline void efx_writeo(struct efx_nic *efx, efx_oword_t *value,
/* Write 64-bit SRAM through the supplied mapping, locking as appropriate. */
static inline void efx_sram_writeq(struct efx_nic *efx, void __iomem *membase,
- efx_qword_t *value, unsigned int index)
+ const efx_qword_t *value, unsigned int index)
{
unsigned int addr = index * sizeof(*value);
unsigned long flags __attribute__ ((unused));
@@ -129,7 +129,7 @@ static inline void efx_sram_writeq(struct efx_nic *efx, void __iomem *membase,
}
/* Write a 32-bit CSR or the last dword of a special 128-bit CSR */
-static inline void efx_writed(struct efx_nic *efx, efx_dword_t *value,
+static inline void efx_writed(struct efx_nic *efx, const efx_dword_t *value,
unsigned int reg)
{
netif_vdbg(efx, hw, efx->net_dev,
@@ -190,8 +190,9 @@ static inline void efx_readd(struct efx_nic *efx, efx_dword_t *value,
}
/* Write a 128-bit CSR forming part of a table */
-static inline void efx_writeo_table(struct efx_nic *efx, efx_oword_t *value,
- unsigned int reg, unsigned int index)
+static inline void
+efx_writeo_table(struct efx_nic *efx, const efx_oword_t *value,
+ unsigned int reg, unsigned int index)
{
efx_writeo(efx, value, reg + index * sizeof(efx_oword_t));
}
@@ -239,8 +240,9 @@ static inline void _efx_writeo_page(struct efx_nic *efx, efx_oword_t *value,
/* Write a page-mapped 32-bit CSR (EVQ_RPTR or the high bits of
* RX_DESC_UPD or TX_DESC_UPD)
*/
-static inline void _efx_writed_page(struct efx_nic *efx, efx_dword_t *value,
- unsigned int reg, unsigned int page)
+static inline void
+_efx_writed_page(struct efx_nic *efx, const efx_dword_t *value,
+ unsigned int reg, unsigned int page)
{
efx_writed(efx, value, EFX_PAGED_REG(page, reg));
}
@@ -256,7 +258,7 @@ static inline void _efx_writed_page(struct efx_nic *efx, efx_dword_t *value,
* collector register.
*/
static inline void _efx_writed_page_locked(struct efx_nic *efx,
- efx_dword_t *value,
+ const efx_dword_t *value,
unsigned int reg,
unsigned int page)
{
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH RESEND net-next 01/16] sfc: Fix lookup of default RX MAC filters when steered using ethtool
From: Ben Hutchings @ 2013-08-22 19:25 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377199326.1703.49.camel@bwh-desktop.uk.level5networks.com>
commit 385904f819e3 ('sfc: Don't use
efx_filter_{build,hash,increment}() for default MAC filters') used the
wrong name to find the index of default RX MAC filters at insertion/
update time. This could result in memory corruption and would in any
case silently fail to update the filter.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/filter.c b/drivers/net/ethernet/sfc/filter.c
index 2a469b2..30d7442 100644
--- a/drivers/net/ethernet/sfc/filter.c
+++ b/drivers/net/ethernet/sfc/filter.c
@@ -675,7 +675,7 @@ s32 efx_filter_insert_filter(struct efx_nic *efx, struct efx_filter_spec *spec,
BUILD_BUG_ON(EFX_FILTER_INDEX_UC_DEF != 0);
BUILD_BUG_ON(EFX_FILTER_INDEX_MC_DEF !=
EFX_FILTER_MC_DEF - EFX_FILTER_UC_DEF);
- rep_index = spec->type - EFX_FILTER_INDEX_UC_DEF;
+ rep_index = spec->type - EFX_FILTER_UC_DEF;
ins_index = rep_index;
spin_lock_bh(&state->lock);
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* Pull request: sfc-next 2013-08-22 v2
From: Ben Hutchings @ 2013-08-22 19:22 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
The following changes since commit 563b44951e76bd23ba3fabcc3a0c708d8f59c5fb:
Merge branch 'openvswitch_vxlan' (2013-08-20 00:16:47 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
for you to fetch changes up to f073dde03b3e8d11050d82f52caaf75fd924e069:
sfc: Make efx_mcdi_init() call efx_mcdi_handle_assertion() (2013-08-21 19:43:09 +0100)
1. Merge of the fix that I just asked you to pull for net. This does
*not* merge in anything else that isn't already in net-next.
2. Refactoring and cleanup in preparation for new hardware support.
Ben.
----------------------------------------------------------------
Ben Hutchings (17):
sfc: Fix lookup of default RX MAC filters when steered using ethtool
Merge branch 'sfc-3.11'
sfc: const-qualify source pointers for MMIO write functions
sfc: Use efx_mcdi_mon() to find efx_mcdi_mon structure from efx_nic
sfc: Move details of a Falcon bug workaround out of ethtool.c
sfc: Move more Falcon-specific code and definitions into falcon.c
sfc: Introduce and use MCDI_DECLARE_BUF macro
sfc: Rationalise MCDI buffer accessors
sfc: Fill out the set of MCDI accessors
sfc: Introduce and use MCDI_CTL_SDU_LEN_MAX_V1 macro for Siena-specific code
sfc: Use proper macros to declare and access MCDI arrays
sfc: Ensure MCDI buffers, but not lengths, are dword aligned
sfc: Add and use MCDI_SET_QWORD() and MCDI_SET_ARRAY_QWORD()
sfc: Move siena_reset_hw() and siena_map_reset_reason() into MCDI module
sfc: Move efx_mcdi_mac_reconfigure() to siena.c and rename
sfc: Collect all MCDI port functions into mcdi_port.c
sfc: Make efx_mcdi_init() call efx_mcdi_handle_assertion()
drivers/net/ethernet/sfc/Makefile | 3 +-
drivers/net/ethernet/sfc/ethtool.c | 23 +-
drivers/net/ethernet/sfc/falcon.c | 563 ++++++++++++++++++++-
drivers/net/ethernet/sfc/falcon_xmac.c | 362 -------------
drivers/net/ethernet/sfc/filter.c | 2 +-
drivers/net/ethernet/sfc/io.h | 18 +-
drivers/net/ethernet/sfc/mcdi.c | 206 ++++----
drivers/net/ethernet/sfc/mcdi.h | 131 +++--
drivers/net/ethernet/sfc/mcdi_mac.c | 130 -----
drivers/net/ethernet/sfc/mcdi_mon.c | 13 +-
drivers/net/ethernet/sfc/mcdi_pcol.h | 4 +-
.../net/ethernet/sfc/{mcdi_phy.c => mcdi_port.c} | 322 +++++++++---
drivers/net/ethernet/sfc/net_driver.h | 2 +
drivers/net/ethernet/sfc/nic.h | 44 +-
drivers/net/ethernet/sfc/phy.h | 17 -
drivers/net/ethernet/sfc/ptp.c | 72 +--
drivers/net/ethernet/sfc/regs.h | 260 ----------
drivers/net/ethernet/sfc/siena.c | 153 ++----
drivers/net/ethernet/sfc/siena_sriov.c | 81 ++-
drivers/net/ethernet/sfc/workarounds.h | 2 -
20 files changed, 1130 insertions(+), 1278 deletions(-)
delete mode 100644 drivers/net/ethernet/sfc/falcon_xmac.c
delete mode 100644 drivers/net/ethernet/sfc/mcdi_mac.c
rename drivers/net/ethernet/sfc/{mcdi_phy.c => mcdi_port.c} (74%)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: Pull request: sfc-next 2013-08-22
From: Ben Hutchings @ 2013-08-22 19:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <20130822.120937.1176691947515308174.davem@davemloft.net>
On Thu, 2013-08-22 at 12:09 -0700, David Miller wrote:
> Ben, this is a completely unreasonable number of patches to submit at
> one time.
>
> Once a series gets past about 15 or 16, the chances of a skilled
> reviewer looking at them all approaches zero.
>
> You have to get the patch count per series down.
>
> I don't care if you have to submit this in chunks or whatever, but
> I'm not pulling this as-is, sorry. It's just too much.
I *am* submitting in chunks, but misjudged the chunk size.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: Pull request: sfc-next 2013-08-22
From: David Miller @ 2013-08-22 19:09 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1377175392.1703.5.camel@bwh-desktop.uk.level5networks.com>
Ben, this is a completely unreasonable number of patches to submit at
one time.
Once a series gets past about 15 or 16, the chances of a skilled
reviewer looking at them all approaches zero.
You have to get the patch count per series down.
I don't care if you have to submit this in chunks or whatever, but
I'm not pulling this as-is, sorry. It's just too much.
^ permalink raw reply
* Re: [PATCH 2/2] Rename nsproxy.pid_ns to nsproxy.pid_ns_for_children
From: Oleg Nesterov @ 2013-08-22 18:56 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Eric W. Biederman, security, linux-kernel, netdev
In-Reply-To: <a5db0606160dbd1412828be0d91a4a115a84abf6.1377196394.git.luto@amacapital.net>
On 08/22, Andy Lutomirski wrote:
>
> nsproxy.pid_ns is *not* the task's pid namespace. The name should clarify
> that.
>
> This makes it more obvious that setns on a pid namespace is weird --
> it won't change the pid namespace shown in procfs.
>
> ...
>
> + * The pid namespace is an exception -- it's accessed using
> + * task_active_pid_ns. The pid namespace here is the
> + * namespace that children will use.
> + *
> * 'count' is the number of tasks holding a reference.
> * The count for each namespace, then, will be the number
> * of nsproxies pointing to it, not the number of tasks.
> @@ -27,7 +31,7 @@ struct nsproxy {
> struct uts_namespace *uts_ns;
> struct ipc_namespace *ipc_ns;
> struct mnt_namespace *mnt_ns;
> - struct pid_namespace *pid_ns;
> + struct pid_namespace *pid_ns_for_children;
> struct net *net_ns;
Personally I agree. ->pid_ns is "strange" and it makes sense to
document and make clear the fact that it became the implicit
argument for clone().
Oleg.
^ permalink raw reply
* Re: [PATCH] net: vlan: inherit addr_assign_type along with dev_addr
From: David Miller @ 2013-08-22 18:49 UTC (permalink / raw)
To: bjorn; +Cc: netdev, kaber
In-Reply-To: <1377174201-20236-1-git-send-email-bjorn@mork.no>
From: Bjørn Mork <bjorn@mork.no>
Date: Thu, 22 Aug 2013 14:23:21 +0200
> A vlan device inheriting a random or set address should reflect
> this in its addr_assign_type.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
I think this might deserve a helper function, I'd not be surprised
if we do MAC address propagation like this in other situations and
they have the same bug.
^ permalink raw reply
* [PATCH 2/2] Rename nsproxy.pid_ns to nsproxy.pid_ns_for_children
From: Andy Lutomirski @ 2013-08-22 18:39 UTC (permalink / raw)
To: Eric W. Biederman, security; +Cc: linux-kernel, netdev, Andy Lutomirski
In-Reply-To: <cover.1377196394.git.luto@amacapital.net>
nsproxy.pid_ns is *not* the task's pid namespace. The name should clarify
that.
This makes it more obvious that setns on a pid namespace is weird --
it won't change the pid namespace shown in procfs.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
include/linux/nsproxy.h | 6 +++++-
kernel/fork.c | 5 +++--
kernel/nsproxy.c | 27 ++++++++++++++-------------
kernel/pid_namespace.c | 4 ++--
4 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index 10e5947..b4ec59d 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -14,6 +14,10 @@ struct fs_struct;
* A structure to contain pointers to all per-process
* namespaces - fs (mount), uts, network, sysvipc, etc.
*
+ * The pid namespace is an exception -- it's accessed using
+ * task_active_pid_ns. The pid namespace here is the
+ * namespace that children will use.
+ *
* 'count' is the number of tasks holding a reference.
* The count for each namespace, then, will be the number
* of nsproxies pointing to it, not the number of tasks.
@@ -27,7 +31,7 @@ struct nsproxy {
struct uts_namespace *uts_ns;
struct ipc_namespace *ipc_ns;
struct mnt_namespace *mnt_ns;
- struct pid_namespace *pid_ns;
+ struct pid_namespace *pid_ns_for_children;
struct net *net_ns;
};
extern struct nsproxy init_nsproxy;
diff --git a/kernel/fork.c b/kernel/fork.c
index e23bb19..bf46287 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1177,7 +1177,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
* don't allow the creation of threads.
*/
if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
- (task_active_pid_ns(current) != current->nsproxy->pid_ns))
+ (task_active_pid_ns(current) !=
+ current->nsproxy->pid_ns_for_children))
return ERR_PTR(-EINVAL);
retval = security_task_create(clone_flags);
@@ -1351,7 +1352,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
if (pid != &init_struct_pid) {
retval = -ENOMEM;
- pid = alloc_pid(p->nsproxy->pid_ns);
+ pid = alloc_pid(p->nsproxy->pid_ns_for_children);
if (!pid)
goto bad_fork_cleanup_io;
}
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index 364ceab..997cbb9 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -29,15 +29,15 @@
static struct kmem_cache *nsproxy_cachep;
struct nsproxy init_nsproxy = {
- .count = ATOMIC_INIT(1),
- .uts_ns = &init_uts_ns,
+ .count = ATOMIC_INIT(1),
+ .uts_ns = &init_uts_ns,
#if defined(CONFIG_POSIX_MQUEUE) || defined(CONFIG_SYSVIPC)
- .ipc_ns = &init_ipc_ns,
+ .ipc_ns = &init_ipc_ns,
#endif
- .mnt_ns = NULL,
- .pid_ns = &init_pid_ns,
+ .mnt_ns = NULL,
+ .pid_ns_for_children = &init_pid_ns,
#ifdef CONFIG_NET
- .net_ns = &init_net,
+ .net_ns = &init_net,
#endif
};
@@ -85,9 +85,10 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
goto out_ipc;
}
- new_nsp->pid_ns = copy_pid_ns(flags, user_ns, tsk->nsproxy->pid_ns);
- if (IS_ERR(new_nsp->pid_ns)) {
- err = PTR_ERR(new_nsp->pid_ns);
+ new_nsp->pid_ns_for_children =
+ copy_pid_ns(flags, user_ns, tsk->nsproxy->pid_ns_for_children);
+ if (IS_ERR(new_nsp->pid_ns_for_children)) {
+ err = PTR_ERR(new_nsp->pid_ns_for_children);
goto out_pid;
}
@@ -100,8 +101,8 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
return new_nsp;
out_net:
- if (new_nsp->pid_ns)
- put_pid_ns(new_nsp->pid_ns);
+ if (new_nsp->pid_ns_for_children)
+ put_pid_ns(new_nsp->pid_ns_for_children);
out_pid:
if (new_nsp->ipc_ns)
put_ipc_ns(new_nsp->ipc_ns);
@@ -174,8 +175,8 @@ void free_nsproxy(struct nsproxy *ns)
put_uts_ns(ns->uts_ns);
if (ns->ipc_ns)
put_ipc_ns(ns->ipc_ns);
- if (ns->pid_ns)
- put_pid_ns(ns->pid_ns);
+ if (ns->pid_ns_for_children)
+ put_pid_ns(ns->pid_ns_for_children);
put_net(ns->net_ns);
kmem_cache_free(nsproxy_cachep, ns);
}
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 6917e8e..601bb36 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -349,8 +349,8 @@ static int pidns_install(struct nsproxy *nsproxy, void *ns)
if (ancestor != active)
return -EINVAL;
- put_pid_ns(nsproxy->pid_ns);
- nsproxy->pid_ns = get_pid_ns(new);
+ put_pid_ns(nsproxy->pid_ns_for_children);
+ nsproxy->pid_ns_for_children = get_pid_ns(new);
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] net: Check the correct namespace when spoofing pid over SCM_RIGHTS
From: Andy Lutomirski @ 2013-08-22 18:39 UTC (permalink / raw)
To: Eric W. Biederman, security; +Cc: linux-kernel, netdev, Andy Lutomirski, stable
In-Reply-To: <cover.1377196394.git.luto@amacapital.net>
This is a security bug.
The follow-up will fix nsproxy to discourage this type of issue from
happening again.
Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
net/core/scm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/scm.c b/net/core/scm.c
index 03795d0..b4da80b 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -54,7 +54,7 @@ static __inline__ int scm_check_creds(struct ucred *creds)
return -EINVAL;
if ((creds->pid == task_tgid_vnr(current) ||
- ns_capable(current->nsproxy->pid_ns->user_ns, CAP_SYS_ADMIN)) &&
+ ns_capable(task_active_pid_ns(current)->user_ns, CAP_SYS_ADMIN)) &&
((uid_eq(uid, cred->uid) || uid_eq(uid, cred->euid) ||
uid_eq(uid, cred->suid)) || nsown_capable(CAP_SETUID)) &&
((gid_eq(gid, cred->gid) || gid_eq(gid, cred->egid) ||
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] Rename nsproxy.pid_ns and fix a related security bug
From: Andy Lutomirski @ 2013-08-22 18:39 UTC (permalink / raw)
To: Eric W. Biederman, security; +Cc: linux-kernel, netdev, Andy Lutomirski
In:
commit 92f28d973cce45ef5823209aab3138eb45d8b349
Author: Eric W. Biederman <ebiederm@xmission.com>
Date: Fri Mar 15 01:03:33 2013 -0700
scm: Require CAP_SYS_ADMIN over the current pidns to spoof pids.
Eric fell for my bogus claim that nsproxy->pid_ns was the current'
process's pid ns. This isn't true.
Let's fix the bug and rename pid_ns so that no one gets this wrong again.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Andy Lutomirski (2):
net: Check the correct namespace when spoofing pid over SCM_RIGHTS
Rename nsproxy.pid_ns to nsproxy.pid_ns_for_children
include/linux/nsproxy.h | 6 +++++-
kernel/fork.c | 5 +++--
kernel/nsproxy.c | 27 ++++++++++++++-------------
kernel/pid_namespace.c | 4 ++--
net/core/scm.c | 2 +-
5 files changed, 25 insertions(+), 19 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH 2/4] net: ethernet: cpsw: add optional third memory region for CONTROL module
From: Daniel Mack @ 2013-08-22 18:37 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: netdev, davem, ujhelyi.m, mugunthanvnm, vaibhav.bedia, d-gerlach,
linux-arm-kernel, linux-omap, devicetree
In-Reply-To: <5216549E.1010305@cogentembedded.com>
Hi Sergei,
On 22.08.2013 20:12, Sergei Shtylyov wrote:
>> @@ -2012,6 +2013,27 @@ static int cpsw_probe(struct platform_device *pdev)
>> goto clean_runtime_disable_ret;
>> }
>>
>> + /* If the control memory region is unspecified, continue without it.
>> + * If it is specified, but we're unable to reserve it, bail. */
>
> According to Documentation/CodingStyle, the networking code's preferred
> style of multi-line comments is this:
>
> /* Bla
> * bla
> */
Ok, thanks. Will fix.
>> + goto clean_runtime_disable_ret;
>> + }
>> + priv->gmii_sel_reg = devm_ioremap(&pdev->dev, res->start,
>> + resource_size(res));
>> + if (!priv->gmii_sel_reg) {
>> + dev_err(priv->dev, "unable to map control i/o region\n");
>> + goto clean_runtime_disable_ret;
>> + }
>
> Why not use devm_ioremap_resource() instead of the above sequence?
Right, that's much nicer. I'll also do that for the devm rework patch.
Thank you for your review!
Daniel
^ permalink raw reply
* [RFC PATCH] bridge sysfs binary file simplification
From: Greg KH @ 2013-08-22 18:30 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: bridge, netdev
From: Greg KH <gregkh@linuxfoundation.org>
Attribute groups now can handle binary sysfs files, so move the bridge
binary sysfs file to be part of the larger bridge group, saving code.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
--
Stephen, this moves the binary sysfs file to be within the bridge
subdirectory, a change from where it is today, right? Was the file
created intentionally outside of the subdir, or was it meant to be
within it?
If tools can't handle this file moving, then never mind with this patch,
it is part of my larger "audit the sysfs binary files" work that I'm
currently doing.
Also note, the sysfs file size is currently set to be 0, is that
intentional?
net/bridge/br_sysfs_br.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 394bb96b..0d5862ea 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -782,11 +782,6 @@ static struct attribute *bridge_attrs[] = {
NULL
};
-static struct attribute_group bridge_group = {
- .name = SYSFS_BRIDGE_ATTR,
- .attrs = bridge_attrs,
-};
-
/*
* Export the forwarding information table as a binary file
* The records are struct __fdb_entry.
@@ -815,10 +810,17 @@ static ssize_t brforward_read(struct file *filp, struct kobject *kobj,
return n;
}
-static struct bin_attribute bridge_forward = {
- .attr = { .name = SYSFS_BRIDGE_FDB,
- .mode = S_IRUGO, },
- .read = brforward_read,
+static BIN_ATTR_RO(brforward, 0);
+
+static struct bin_attribute *bridge_bin_attrs[] = {
+ &bin_attr_brforward,
+ NULL,
+};
+
+static struct attribute_group bridge_group = {
+ .name = SYSFS_BRIDGE_ATTR,
+ .attrs = bridge_attrs,
+ .bin_attrs = bridge_bin_attrs,
};
/*
@@ -845,22 +847,13 @@ int br_sysfs_addbr(struct net_device *dev)
goto out1;
}
- err = sysfs_create_bin_file(brobj, &bridge_forward);
- if (err) {
- pr_info("%s: can't create attribute file %s/%s\n",
- __func__, dev->name, bridge_forward.attr.name);
- goto out2;
- }
-
br->ifobj = kobject_create_and_add(SYSFS_BRIDGE_PORT_SUBDIR, brobj);
if (!br->ifobj) {
pr_info("%s: can't add kobject (directory) %s/%s\n",
__func__, dev->name, SYSFS_BRIDGE_PORT_SUBDIR);
- goto out3;
+ goto out2;
}
return 0;
- out3:
- sysfs_remove_bin_file(&dev->dev.kobj, &bridge_forward);
out2:
sysfs_remove_group(&dev->dev.kobj, &bridge_group);
out1:
@@ -874,6 +867,5 @@ void br_sysfs_delbr(struct net_device *dev)
struct net_bridge *br = netdev_priv(dev);
kobject_put(br->ifobj);
- sysfs_remove_bin_file(kobj, &bridge_forward);
sysfs_remove_group(kobj, &bridge_group);
}
^ permalink raw reply related
* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Johannes Berg @ 2013-08-22 18:18 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, Jesse Gross
In-Reply-To: <CALnjE+rEOMmyFQ0Nx3-O-Cvu8yePVBg7x8vgpGUOAAnYv3KG4Q@mail.gmail.com>
On Thu, 2013-08-22 at 11:10 -0700, Pravin Shelar wrote:
> > By the way - why? This just means that netlink will allocate another
> > lock to lock it all, so it's not a very useful change?
> >
> This replaces global genl-lock with per-socket lock which allows
> parallel operation in parallel-genl-families and they are not blocked
> due to other unrelated genl-family operations.
I don't think so? It replaces the genl lock as cb_mutex with a per
*kernel* socket lock, so really just one per network namespace. That's
not much of an improvement really.
johannes
^ permalink raw reply
* Re: [PATCH 2/4] net: ethernet: cpsw: add optional third memory region for CONTROL module
From: Sergei Shtylyov @ 2013-08-22 18:12 UTC (permalink / raw)
To: Daniel Mack
Cc: netdev, davem, ujhelyi.m, mugunthanvnm, vaibhav.bedia, d-gerlach,
linux-arm-kernel, linux-omap, devicetree
In-Reply-To: <1377171448-27924-3-git-send-email-zonque@gmail.com>
Hello.
On 08/22/2013 03:37 PM, Daniel Mack wrote:
> At least the AM33xx SoC has a control module register to configure
> details such as the hardware ethernet interface mode.
> I'm not sure whether all SoCs which feature the cpsw block have such a
> register, so that third memory region is considered optional for now.
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
> Documentation/devicetree/bindings/net/cpsw.txt | 5 ++++-
> drivers/net/ethernet/ti/cpsw.c | 22 ++++++++++++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 05d660e..4e5ca54 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
[...]
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 63feaae..4855d8e 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
[...]
> @@ -2012,6 +2013,27 @@ static int cpsw_probe(struct platform_device *pdev)
> goto clean_runtime_disable_ret;
> }
>
> + /* If the control memory region is unspecified, continue without it.
> + * If it is specified, but we're unable to reserve it, bail. */
According to Documentation/CodingStyle, the networking code's preferred
style of multi-line comments is this:
/* Bla
* bla
*/
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + if (!res) {
> + dev_err(priv->dev, "error getting control i/o resource\n");
> + goto no_gmii_sel;
> + }
> + if (!devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
> + ndev->name)) {
Not dev_name(&pdev->dev)?
> + dev_err(priv->dev, "failed request control i/o region\n");
> + ret = -ENXIO;
Rather -EBUSY.
> + goto clean_runtime_disable_ret;
> + }
> + priv->gmii_sel_reg = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!priv->gmii_sel_reg) {
> + dev_err(priv->dev, "unable to map control i/o region\n");
> + goto clean_runtime_disable_ret;
> + }
Why not use devm_ioremap_resource() instead of the above sequence?
> +
> +no_gmii_sel:
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Pravin Shelar @ 2013-08-22 18:10 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jesse Gross
In-Reply-To: <1377193906.14110.27.camel@jlt4.sipsolutions.net>
On Thu, Aug 22, 2013 at 10:51 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
>> On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
>> >> In case of genl-family with parallel ops off, dumpif() callback
>> >> is expected to run under genl_lock, But commit def3117493eafd9df
>> >> (genl: Allow concurrent genl callbacks.) changed this behaviour
>> >> where only first dumpit() op was called under genl-lock.
>> >> For subsequent dump, only nlk->cb_lock was taken.
>> >> Following patch fixes it by defining locked dumpit() and done()
>> >> callback which takes care of genl-locking.
>> >
>> > As I've commented over in the other thread, I really think this is the
>> > wrong thing to do. It was never the case that dumpit() was actually
>> > locked under genl_lock(), and adding it doesn't really help anyone.
>> >
>> If you look at commit def3117493eafd9df , it removes assignment of
>> cb_mutex to genl_mutex. Thats how genl-dump operation was called under
>> genl_mutex.
>
> By the way - why? This just means that netlink will allocate another
> lock to lock it all, so it's not a very useful change?
>
This replaces global genl-lock with per-socket lock which allows
parallel operation in parallel-genl-families and they are not blocked
due to other unrelated genl-family operations.
^ permalink raw reply
* Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.
From: Pravin Shelar @ 2013-08-22 18:04 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jesse Gross
In-Reply-To: <1377193772.14110.25.camel@jlt4.sipsolutions.net>
On Thu, Aug 22, 2013 at 10:49 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-08-22 at 10:44 -0700, Pravin Shelar wrote:
>
>> > This may be more correct than my patch, but I'm not sure it's worth
>> > spending the memory. Is there going to be any generic netlink family
>> > that actually puts operations into a different module than the family
>> > itself? I doubt that.
>> >
>> I tried to do same, but I do not have access to ops in
>> genl_lock_done() function. therefore I decided to store direct pointer
>> to module in ops struct.
>
> You mean the family? I still don't like this, it'll be a massive
> increase in memory for something like nl80211 that has a lot of
> operations. In fact I think we should get rid of being allowed to
> register single ops and just force a single array to remove the linked
> list as well.
>
Sorry, I meant I do not have access to genl-family in genl_lock_done().
I am using c.data to store ops pointer. Thats why module pointer was
store in ops.
But considering we do not need to hold ref on genl, I can move module
pointer to family.
>> >> + c.module = THIS_MODULE;
>> >
>> > THIS_MODULE here is useless, this code is always built-in.
>> >
>> >> + if (!try_module_get(ops->module))
>> >> + return -EPROTONOSUPPORT;
>> >
>> > Why open-code it? You can just point c.module to the ops module here as
>> > well (because generic netlink is built-in) and save yourself the
>> > try_module_get stuff.
>> >
>>
>> In locked genl case genl-dump operation has call graph as follows:
>> netlink_dump() -> genl_lock_dumpit() -> ops->dump()
>> Therefore I need to take ref on genl module and ops->module.
>
> There's no "genl module", generic netlink is always built in. That was
> my point, you don't need to hold any reference to the "genl module"
> since it doesn't exist, so you can just point to the ops (family)
> module.
>
ok, this make sense.
I will send updated patch.
^ permalink raw reply
* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Johannes Berg @ 2013-08-22 17:51 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, Jesse Gross
In-Reply-To: <CALnjE+qgZ_UZ4GdiZ4UCzfTLn-OveoHtE28uQZokz2kPD4srXQ@mail.gmail.com>
On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
> On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
> >> In case of genl-family with parallel ops off, dumpif() callback
> >> is expected to run under genl_lock, But commit def3117493eafd9df
> >> (genl: Allow concurrent genl callbacks.) changed this behaviour
> >> where only first dumpit() op was called under genl-lock.
> >> For subsequent dump, only nlk->cb_lock was taken.
> >> Following patch fixes it by defining locked dumpit() and done()
> >> callback which takes care of genl-locking.
> >
> > As I've commented over in the other thread, I really think this is the
> > wrong thing to do. It was never the case that dumpit() was actually
> > locked under genl_lock(), and adding it doesn't really help anyone.
> >
> If you look at commit def3117493eafd9df , it removes assignment of
> cb_mutex to genl_mutex. Thats how genl-dump operation was called under
> genl_mutex.
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
johannes
^ permalink raw reply
* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Johannes Berg @ 2013-08-22 17:51 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, Jesse Gross
In-Reply-To: <CALnjE+qgZ_UZ4GdiZ4UCzfTLn-OveoHtE28uQZokz2kPD4srXQ@mail.gmail.com>
On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
> On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
> >> In case of genl-family with parallel ops off, dumpif() callback
> >> is expected to run under genl_lock, But commit def3117493eafd9df
> >> (genl: Allow concurrent genl callbacks.) changed this behaviour
> >> where only first dumpit() op was called under genl-lock.
> >> For subsequent dump, only nlk->cb_lock was taken.
> >> Following patch fixes it by defining locked dumpit() and done()
> >> callback which takes care of genl-locking.
> >
> > As I've commented over in the other thread, I really think this is the
> > wrong thing to do. It was never the case that dumpit() was actually
> > locked under genl_lock(), and adding it doesn't really help anyone.
> >
> If you look at commit def3117493eafd9df , it removes assignment of
> cb_mutex to genl_mutex. Thats how genl-dump operation was called under
> genl_mutex.
> This patch simply restores that locking.
Huh, ok, so you actually caused all the dump locking bugs in
nl80211! :-)
johannes
^ permalink raw reply
* Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.
From: Johannes Berg @ 2013-08-22 17:49 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, Jesse Gross
In-Reply-To: <CALnjE+oAO_1VQVPp1yq9UnHU0fyRAp7vBB0huaMVxRWMsPvK0w@mail.gmail.com>
On Thu, 2013-08-22 at 10:44 -0700, Pravin Shelar wrote:
> > This may be more correct than my patch, but I'm not sure it's worth
> > spending the memory. Is there going to be any generic netlink family
> > that actually puts operations into a different module than the family
> > itself? I doubt that.
> >
> I tried to do same, but I do not have access to ops in
> genl_lock_done() function. therefore I decided to store direct pointer
> to module in ops struct.
You mean the family? I still don't like this, it'll be a massive
increase in memory for something like nl80211 that has a lot of
operations. In fact I think we should get rid of being allowed to
register single ops and just force a single array to remove the linked
list as well.
> >> + c.module = THIS_MODULE;
> >
> > THIS_MODULE here is useless, this code is always built-in.
> >
> >> + if (!try_module_get(ops->module))
> >> + return -EPROTONOSUPPORT;
> >
> > Why open-code it? You can just point c.module to the ops module here as
> > well (because generic netlink is built-in) and save yourself the
> > try_module_get stuff.
> >
>
> In locked genl case genl-dump operation has call graph as follows:
> netlink_dump() -> genl_lock_dumpit() -> ops->dump()
> Therefore I need to take ref on genl module and ops->module.
There's no "genl module", generic netlink is always built in. That was
my point, you don't need to hold any reference to the "genl module"
since it doesn't exist, so you can just point to the ops (family)
module.
johannes
^ permalink raw reply
* Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.
From: Pravin Shelar @ 2013-08-22 17:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jesse Gross
In-Reply-To: <1377157242.14110.21.camel@jlt4.sipsolutions.net>
On Thu, Aug 22, 2013 at 12:40 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
>> netlink dump operations take module as parameter to hold
>> reference for entire netlink dump duration.
>> Currently it holds ref only on genl module which is not correct
>> when we use ops registered to genl from another module.
>> Following patch adds module pointer to genl_ops so that netlink
>> can hold ref count on it.
>>
>> CC: Jesse Gross <jesse@nicira.com>
>> CC: Johannes Berg <johannes.berg@intel.com>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> ---
>> include/net/genetlink.h | 21 ++++++++++++++++++---
>> net/netlink/genetlink.c | 39 ++++++++++++++++++++++++---------------
>> 2 files changed, 42 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
>> index 93024a4..7f57b2c 100644
>> --- a/include/net/genetlink.h
>> +++ b/include/net/genetlink.h
>> @@ -119,13 +119,28 @@ struct genl_ops {
>> struct netlink_callback *cb);
>> int (*done)(struct netlink_callback *cb);
>> struct list_head ops_list;
>> + struct module *module;
>> };
>
> This may be more correct than my patch, but I'm not sure it's worth
> spending the memory. Is there going to be any generic netlink family
> that actually puts operations into a different module than the family
> itself? I doubt that.
>
I tried to do same, but I do not have access to ops in
genl_lock_done() function. therefore I decided to store direct pointer
to module in ops struct.
>> @@ -605,14 +610,18 @@ static int genl_family_rcv_msg(struct genl_family *family,
>> genl_unlock();
>> c.data = ops;
>> c.dump = genl_lock_dumpit;
>> - if (ops->done)
>> - c.done = genl_lock_done;
>> + c.done = genl_lock_done;
>> + c.module = THIS_MODULE;
>
> THIS_MODULE here is useless, this code is always built-in.
>
>> + if (!try_module_get(ops->module))
>> + return -EPROTONOSUPPORT;
>
> Why open-code it? You can just point c.module to the ops module here as
> well (because generic netlink is built-in) and save yourself the
> try_module_get stuff.
>
In locked genl case genl-dump operation has call graph as follows:
netlink_dump() -> genl_lock_dumpit() -> ops->dump()
Therefore I need to take ref on genl module and ops->module.
In case of parallel_ops been true, genl_lock_dumpit() is not used so I
have directly set c.module to ops->module.
I know this code is bit complicated, but I could not find way to simplify it.
^ permalink raw reply
* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Pravin Shelar @ 2013-08-22 17:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jesse Gross
In-Reply-To: <1377156980.14110.16.camel@jlt4.sipsolutions.net>
On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
>> In case of genl-family with parallel ops off, dumpif() callback
>> is expected to run under genl_lock, But commit def3117493eafd9df
>> (genl: Allow concurrent genl callbacks.) changed this behaviour
>> where only first dumpit() op was called under genl-lock.
>> For subsequent dump, only nlk->cb_lock was taken.
>> Following patch fixes it by defining locked dumpit() and done()
>> callback which takes care of genl-locking.
>
> As I've commented over in the other thread, I really think this is the
> wrong thing to do. It was never the case that dumpit() was actually
> locked under genl_lock(), and adding it doesn't really help anyone.
>
If you look at commit def3117493eafd9df , it removes assignment of
cb_mutex to genl_mutex. Thats how genl-dump operation was called under
genl_mutex.
This patch simply restores that locking.
^ permalink raw reply
* [RFC] iproute2: display devconf info
From: Stephen Hemminger @ 2013-08-22 17:23 UTC (permalink / raw)
To: netdev
Would this be useful, it allows displaying devconf information with ip command.
Open to suggestions about format, and how to condense the display.
$ ip -d addr show dev wlan0
3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
ipv4 conf:
forwarding:1
accept redirects:1
secure redirects:1
send redirects:1
shared media:1
accept source route:1
ipv6 conf:
hoplimit:64
mtu6:1500
accept ra:2
accept redirects:1
autoconf:1
dad transmits:1
rtr solicits:3
rtr solicit interval:4000
rtr solicit delay:1000
use tempaddr:2
temp valid lft:604800
temp prefered lft:86400
regen max retry:3
max desync factor:600
max addresses:16
accept ra defrtr:1
accept ra pinfo:1
accept ra rtr pref:1
rtr probe interval:60000
accept dad:1
inet 192.168.1.189/24 brd 192.168.1.255 scope global wlan0
valid_lft forever preferred_lft forever
inet6 fe80::c685:8ff:feca:841f/64 scope link
valid_lft forever preferred_lft forever
---
ip/ipaddress.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 123 insertions(+)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 1c3e4da..f49e246 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -386,6 +386,126 @@ static void print_link_stats(FILE *fp, const struct rtnl_link_stats *s)
}
}
+static const char *ipv4_devconf[] = {
+ "forwarding",
+ "mc forwarding",
+ "proxy arp",
+ "accept redirects",
+ "secure redirects",
+ "send redirects",
+ "shared media",
+ "rp filter",
+ "accept source route",
+ "bootp relay",
+ "log martians",
+ "tag",
+ "arpfilter",
+ "medium id",
+ "noxfrm",
+ "nopolicy",
+ "force igmp version",
+ "arp announce",
+ "arp ignore",
+ "promote secondaries",
+ "arp accept",
+ "arp notify",
+ "accept local",
+ "src vmark",
+ "proxy arp pvlan",
+ "route localnet",
+};
+
+static void print_inet_conf(FILE *fp, struct rtattr *ininfo)
+{
+ struct rtattr *rta, *tb[IFLA_INET_MAX+1];
+
+ parse_rtattr_nested(tb, IFLA_INET_MAX, ininfo);
+
+ if ((rta = tb[IFLA_INET_CONF])) {
+ const __u32 *conf = (__u32 *)RTA_DATA(rta);
+ unsigned i;
+
+ fprintf(fp, "\n ipv4 conf:");
+ for (i = 0; i < rta->rta_len && i < ARRAY_SIZE(ipv4_devconf); i++) {
+ if (conf[i] != 0)
+ fprintf(fp, "\n %s:%u",
+ ipv4_devconf[i], conf[i]);
+ }
+ }
+}
+
+static const char *ipv6_devconf[] = {
+ "forwarding",
+ "hoplimit",
+ "mtu6",
+ "accept ra",
+ "accept redirects",
+ "autoconf",
+ "dad transmits",
+ "rtr solicits",
+ "rtr solicit interval",
+ "rtr solicit delay",
+ "use tempaddr",
+ "temp valid lft",
+ "temp prefered lft",
+ "regen max retry",
+ "max desync factor",
+ "max addresses",
+ "force mld version",
+ "accept ra defrtr",
+ "accept ra pinfo",
+ "accept ra rtr pref",
+ "rtr probe interval",
+ "accept ra rt info max plen",
+ "proxy ndp",
+ "optimistic dad",
+ "accept source route",
+ "mc forwarding",
+ "disable ipv6",
+ "accept dad",
+ "force tllao",
+ "ndisc notify",
+};
+
+static void print_inet6_conf(FILE *fp, struct rtattr *in6info)
+{
+ struct rtattr *rta, *tb[IFLA_INET6_MAX+1];
+
+ parse_rtattr_nested(tb, IFLA_INET6_MAX, in6info);
+
+ if ((rta = tb[IFLA_INET6_CONF])) {
+ const __u32 *conf = (__u32 *)RTA_DATA(rta);
+ unsigned i;
+
+ fprintf(fp, "\n ipv6 conf:");
+ for (i = 0; i < rta->rta_len && i < ARRAY_SIZE(ipv6_devconf); i++) {
+ if (conf[i] != 0)
+ fprintf(fp, "\n %s:%u",
+ ipv6_devconf[i], conf[i]);
+ }
+ }
+}
+
+static void print_afinfo(FILE *fp, struct rtattr *afinfo)
+{
+ struct rtattr *tb[AF_MAX+1];
+
+ if (afinfo->rta_type != IFLA_AF_SPEC) {
+ fprintf(stderr, "BUG: rta type is %d\n", afinfo->rta_type);
+ return;
+ }
+
+ parse_rtattr_nested(tb, AF_MAX, afinfo);
+
+ if (tb[AF_INET] &&
+ (!filter.family || filter.family == AF_INET))
+ print_inet_conf(fp, tb[AF_INET]);
+
+ if (tb[AF_INET6] &&
+ (!filter.family || filter.family == AF_INET))
+ print_inet6_conf(fp, tb[AF_INET6]);
+}
+
int print_linkinfo(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
{
@@ -509,6 +629,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
print_link_stats(fp, RTA_DATA(tb[IFLA_STATS]));
}
+ if (show_details && tb[IFLA_AF_SPEC])
+ print_afinfo(fp, tb[IFLA_AF_SPEC]);
+
if (do_link && tb[IFLA_VFINFO_LIST] && tb[IFLA_NUM_VF]) {
struct rtattr *i, *vflist = tb[IFLA_VFINFO_LIST];
int rem = RTA_PAYLOAD(vflist);
--
1.7.10.4
^ permalink raw reply related
* Re: Null pointer dereference in nl80211_set_reg
From: Ben Hutchings @ 2013-08-22 16:04 UTC (permalink / raw)
To: udo; +Cc: Linux Kernel Mailing List, Linux Network Mailing List
In-Reply-To: <20130822174121.18441b7f@x220>
On Thu, 2013-08-22 at 17:41 +0200, Udo Steinberg wrote:
> Hi all,
>
> I'm running Linux 3.10.7 and have encountered the following NULL pointer
> dereference. So far it has only occurred once, so I cannot reproduce the
> circumstances that cause the problem.
>
> Please keep me on CC: as I'm not subscribed to LKML.
[...]
Probably fixed by:
commit c319d50bfcf678c2857038276d9fab3c6646f3bf
Author: Johannes Berg <johannes.berg@intel.com>
Date: Tue Jul 30 22:34:28 2013 +0200
nl80211: fix another nl80211_fam.attrbuf race
which went into 3.10.8.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox