Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] ptp: measure the time offset between PHC and system clock
From: Richard Cochran @ 2013-09-14 14:31 UTC (permalink / raw)
  To: Dong Zhu; +Cc: Rob Landley, netdev, linux-doc, linux-kernel
In-Reply-To: <20130914080306.GC23682@zhudong.nay.redhat.com>

On Sat, Sep 14, 2013 at 04:03:06PM +0800, Dong Zhu wrote:
> This patch add a method into testptp.c to measure the time offset
> between phc and system clock through the ioctl PTP_SYS_OFFSET.
> 

This is a nice addition to the testptp program. I do have a few
comments, below.

First off, the subject line should mention testptp. How about this?

    [PATCH] ptp: add the PTP_SYS_OFFSET ioctl to the testptp program

> Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> ---
>  Documentation/ptp/testptp.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ptp/testptp.c b/Documentation/ptp/testptp.c
> index f59ded0..72bb030 100644
> --- a/Documentation/ptp/testptp.c
> +++ b/Documentation/ptp/testptp.c
> @@ -112,6 +112,7 @@ static void usage(char *progname)
>  		" -f val     adjust the ptp clock frequency by 'val' ppb\n"
>  		" -g         get the ptp clock time\n"
>  		" -h         prints this message\n"
> +		" -k val     measure the time offset between PHC and system clock\n"

The help message should tell the user what 'val' is.

>  		" -p val     enable output with a period of 'val' nanoseconds\n"
>  		" -P val     enable or disable (val=1|0) the system clock PPS\n"
>  		" -s         set the ptp clock time from the system time\n"
> @@ -133,8 +134,12 @@ int main(int argc, char *argv[])
>  	struct itimerspec timeout;
>  	struct sigevent sigevent;
>  
> +	struct ptp_clock_time *pct;
> +	struct ptp_sys_offset *sysoff;
> +
> +
>  	char *progname;
> -	int c, cnt, fd;
> +	int i, c, cnt, fd;
>  
>  	char *device = DEVICE;
>  	clockid_t clkid;
> @@ -144,6 +149,8 @@ int main(int argc, char *argv[])
>  	int extts = 0;
>  	int gettime = 0;
>  	int oneshot = 0;
> +	int offset = 0;
> +	int n_samples = 0;
>  	int periodic = 0;
>  	int perout = -1;
>  	int pps = -1;
> @@ -151,7 +158,7 @@ int main(int argc, char *argv[])
>  
>  	progname = strrchr(argv[0], '/');
>  	progname = progname ? 1+progname : argv[0];
> -	while (EOF != (c = getopt(argc, argv, "a:A:cd:e:f:ghp:P:sSt:v"))) {
> +	while (EOF != (c = getopt(argc, argv, "a:A:cd:e:f:ghk:p:P:sSt:v"))) {
>  		switch (c) {
>  		case 'a':
>  			oneshot = atoi(optarg);
> @@ -174,6 +181,10 @@ int main(int argc, char *argv[])
>  		case 'g':
>  			gettime = 1;
>  			break;
> +		case 'k':
> +			offset = 1;
> +			n_samples = atoi(optarg);
> +			break;
>  		case 'p':
>  			perout = atoi(optarg);
>  			break;
> @@ -376,6 +387,31 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> +	if (offset) {
> +		sysoff = calloc(1, sizeof(*sysoff));
> +		if (!sysoff) {
> +			perror("calloc");
> +			return -1;
> +		}
> +		sysoff->n_samples = n_samples;
> +
> +		if (ioctl(fd, PTP_SYS_OFFSET, sysoff))
> +			perror("PTP_SYS_OFFSET");
> +		else
> +			puts("time offset between PHC and
> +					 system clock request okay");
> +
> +		pct = &sysoff->ts[0];
> +		for (i = 0; i < sysoff->n_samples; i++, pct++) {
> +			printf("system time: %ld.%ld\n", pct->sec, pct->nsec);
> +			pct++;
> +			printf("phc    time: %ld.%ld\n\n", pct->sec, pct->nsec);
                                                    ^^^^
I think the output would look nicer with only one newline. After all,
each measurement is a {sys,phc,sys} triplet and not a {sys,phc} pair.

> +		}
> +		printf("system time: %ld.%ld\n", pct->sec, pct->nsec);
> +
> +		free(sysoff);
> +	}
> +

Thanks,
Richard

^ permalink raw reply

* [PATCH net-next 0/2] bridge: fix NULL pointer deref of br_port_get_rcu
From: Hong Zhiguo @ 2013-09-14 14:42 UTC (permalink / raw)
  To: netdev; +Cc: davem, eric.dumazet, vyasevic, Hong Zhiguo

From: Hong Zhiguo <zhiguohong@tencent.com>

I got an Oops on my box when br_handle_frame is called between these
2 lines of del_nbp:
	dev->priv_flags &= ~IFF_BRIDGE_PORT;
	/* --> br_handle_frame is called at this time */
	netdev_rx_handler_unregister(dev);

In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
without check but br_port_get_rcu(dev) returns NULL if:
	!(dev->priv_flags & IFF_BRIDGE_PORT)

Eric Dumazet pointed out the testing of IFF_BRIDGE_PORT is not necessary
here since we're in rcu_read_lock and we have synchronize_net() in
netdev_rx_handler_unregister. So removed the testing of
IFF_BRIDGE_PORT.

I tested the fix on my box with script doing "brctl addif" and "brctl
delif" repeatedly while a lot of broadcast frame present on the LAN.
I added msleep in del_nbp between setting of priv_flags and unregister
so it's easy to reproduce the oops without the fix.

I want to remove the NULL checks following call to br_port_get_rcu in
Br_netfilter and ebtables code. Could someone confirm that's OK?

The Oops(some lines omitted):
BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
Oops: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff8150901d>]  [<ffffffff8150901d>] br_handle_frame+0xed/0x230
RSP: 0018:ffff880030403c10  EFLAGS: 00010286
Stack:
 ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
 ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
 ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
Call Trace:
 <IRQ>
 [<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
 [<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880

Hong Zhiguo (2):
  bridge: use br_port_get_rtnl within rtnl lock
  bridge: fix NULL pointer deref of br_port_get_rcu

 net/bridge/br_netlink.c | 4 ++--
 net/bridge/br_private.h | 7 ++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

-- 
1.8.1.2

^ permalink raw reply

* [PATCH net-next 1/2] bridge: use br_port_get_rtnl within rtnl lock
From: Hong Zhiguo @ 2013-09-14 14:42 UTC (permalink / raw)
  To: netdev; +Cc: davem, eric.dumazet, vyasevic, Hong Zhiguo
In-Reply-To: <1379169748-767-1-git-send-email-zhiguohong@tencent.com>

From: Hong Zhiguo <zhiguohong@tencent.com>

current br_port_get_rcu is problematic in bridging path
(NULL deref). Change these calls in netlink path first.

Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
 net/bridge/br_netlink.c | 4 ++--
 net/bridge/br_private.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index b9259ef..e74ddc1 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -207,7 +207,7 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 	       struct net_device *dev, u32 filter_mask)
 {
 	int err = 0;
-	struct net_bridge_port *port = br_port_get_rcu(dev);
+	struct net_bridge_port *port = br_port_get_rtnl(dev);
 
 	/* not a bridge port and  */
 	if (!port && !(filter_mask & RTEXT_FILTER_BRVLAN))
@@ -451,7 +451,7 @@ static size_t br_get_link_af_size(const struct net_device *dev)
 	struct net_port_vlans *pv;
 
 	if (br_port_exists(dev))
-		pv = nbp_get_vlan_info(br_port_get_rcu(dev));
+		pv = nbp_get_vlan_info(br_port_get_rtnl(dev));
 	else if (dev->priv_flags & IFF_EBRIDGE)
 		pv = br_get_vlan_info((struct net_bridge *)netdev_priv(dev));
 	else
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 598cb0b..49fb43e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -208,7 +208,7 @@ static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *d
 	return br_port_exists(dev) ? port : NULL;
 }
 
-static inline struct net_bridge_port *br_port_get_rtnl(struct net_device *dev)
+static inline struct net_bridge_port *br_port_get_rtnl(const struct net_device *dev)
 {
 	return br_port_exists(dev) ?
 		rtnl_dereference(dev->rx_handler_data) : NULL;
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu
From: Hong Zhiguo @ 2013-09-14 14:42 UTC (permalink / raw)
  To: netdev; +Cc: davem, eric.dumazet, vyasevic, Hong Zhiguo
In-Reply-To: <1379169748-767-1-git-send-email-zhiguohong@tencent.com>

From: Hong Zhiguo <zhiguohong@tencent.com>

The NULL deref happens when br_handle_frame is called between these
2 lines of del_nbp:
	dev->priv_flags &= ~IFF_BRIDGE_PORT;
	/* --> br_handle_frame is called at this time */
	netdev_rx_handler_unregister(dev);

In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
without check but br_port_get_rcu(dev) returns NULL if:
	!(dev->priv_flags & IFF_BRIDGE_PORT)

Eric Dumazet pointed out the testing of IFF_BRIDGE_PORT is not necessary
here since we're in rcu_read_lock and we have synchronize_net() in
netdev_rx_handler_unregister. So remove the testing of IFF_BRIDGE_PORT
and by the previous patch, make sure br_port_get_rcu is called in
bridging code.

Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
 net/bridge/br_private.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 49fb43e..1aaca0e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -202,10 +202,7 @@ struct net_bridge_port
 
 static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
 {
-	struct net_bridge_port *port =
-			rcu_dereference_rtnl(dev->rx_handler_data);
-
-	return br_port_exists(dev) ? port : NULL;
+	return rcu_dereference(dev->rx_handler_data);
 }
 
 static inline struct net_bridge_port *br_port_get_rtnl(const struct net_device *dev)
-- 
1.8.1.2

^ permalink raw reply related

* Re: [PATCH] ptp: add the PTP_SYS_OFFSET ioctl to the testptp program clock
From: Dong Zhu @ 2013-09-14 15:39 UTC (permalink / raw)
  To: Richard Cochran; +Cc: Rob Landley, netdev, linux-doc, linux-kernel
In-Reply-To: <20130914143144.GA4206@netboy>

On Sat, Sep 14, 2013 at 04:31:46PM +0200, Richard Cochran wrote:
> On Sat, Sep 14, 2013 at 04:03:06PM +0800, Dong Zhu wrote:
> > This patch add a method into testptp.c to measure the time offset
> > between phc and system clock through the ioctl PTP_SYS_OFFSET.
> > 
> 
> This is a nice addition to the testptp program. I do have a few
> comments, below.
> 

Thanks very much for your comments, I have modified the patch as below,
Cuold you have a look at it again ? Any comments would be appreciated.

>From 655b45785a85599d5fff5eb3b8d9b49b72f2991f Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Sat, 14 Sep 2013 23:32:14 +0800

This patch add a method into testptp.c to measure the time offset
between phc and system clock through the ioctl PTP_SYS_OFFSET.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
---
 Documentation/ptp/testptp.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/ptp/testptp.c b/Documentation/ptp/testptp.c
index f59ded0..8acdc70 100644
--- a/Documentation/ptp/testptp.c
+++ b/Documentation/ptp/testptp.c
@@ -112,6 +112,8 @@ static void usage(char *progname)
 		" -f val     adjust the ptp clock frequency by 'val' ppb\n"
 		" -g         get the ptp clock time\n"
 		" -h         prints this message\n"
+		" -k val     measure the time offset between phc and system clock "
+		"for 'val' times (Maximum 25)\n"
 		" -p val     enable output with a period of 'val' nanoseconds\n"
 		" -P val     enable or disable (val=1|0) the system clock PPS\n"
 		" -s         set the ptp clock time from the system time\n"
@@ -133,8 +135,12 @@ int main(int argc, char *argv[])
 	struct itimerspec timeout;
 	struct sigevent sigevent;
 
+	struct ptp_clock_time *pct;
+	struct ptp_sys_offset *sysoff;
+
+
 	char *progname;
-	int c, cnt, fd;
+	int i, c, cnt, fd;
 
 	char *device = DEVICE;
 	clockid_t clkid;
@@ -144,6 +150,8 @@ int main(int argc, char *argv[])
 	int extts = 0;
 	int gettime = 0;
 	int oneshot = 0;
+	int offset = 0;
+	int n_samples = 0;
 	int periodic = 0;
 	int perout = -1;
 	int pps = -1;
@@ -151,7 +159,7 @@ int main(int argc, char *argv[])
 
 	progname = strrchr(argv[0], '/');
 	progname = progname ? 1+progname : argv[0];
-	while (EOF != (c = getopt(argc, argv, "a:A:cd:e:f:ghp:P:sSt:v"))) {
+	while (EOF != (c = getopt(argc, argv, "a:A:cd:e:f:ghk:p:P:sSt:v"))) {
 		switch (c) {
 		case 'a':
 			oneshot = atoi(optarg);
@@ -174,6 +182,10 @@ int main(int argc, char *argv[])
 		case 'g':
 			gettime = 1;
 			break;
+		case 'k':
+			offset = 1;
+			n_samples = atoi(optarg);
+			break;
 		case 'p':
 			perout = atoi(optarg);
 			break;
@@ -376,6 +388,30 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (offset) {
+		sysoff = calloc(1, sizeof(*sysoff));
+		if (!sysoff) {
+			perror("calloc");
+			return -1;
+		}
+		sysoff->n_samples = n_samples;
+
+		if (ioctl(fd, PTP_SYS_OFFSET, sysoff))
+			perror("PTP_SYS_OFFSET");
+		else
+			puts("phc and system clock time offset request okay");
+
+		pct = &sysoff->ts[0];
+		for (i = 0; i < sysoff->n_samples; i++, pct++) {
+			printf("system time: %ld.%ld\n", pct->sec, pct->nsec);
+			pct++;
+			printf("phc    time: %ld.%ld\n", pct->sec, pct->nsec);
+		}
+		printf("system time: %ld.%ld\n", pct->sec, pct->nsec);
+
+		free(sysoff);
+	}
+
 	close(fd);
 	return 0;
 }
-- 
1.7.11.7

-- 
Best Regards,
Dong Zhu

^ permalink raw reply related

* Re: [PATCH net 0/4] bridge: Fix problems around the PVID
From: Toshiaki Makita @ 2013-09-14 15:42 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: David Miller, makita.toshiaki, vyasevic, netdev,
	Fernando Luis Vazquez Cao, Patrick McHardy
In-Reply-To: <20130913152114.GD695@redhat.com>

On Fri, 2013-09-13 at 17:21 +0200, Veaceslav Falico wrote:
> On Fri, Sep 13, 2013 at 09:06:53PM +0900, Toshiaki Makita wrote:
> >On Thu, 2013-09-12 at 16:00 -0400, David Miller wrote:
> >> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >> Date: Tue, 10 Sep 2013 19:27:54 +0900
> >>
> >> > There seem to be some undesirable behaviors related with PVID.
> >> > 1. It has no effect assigning PVID to a port. PVID cannot be applied
> >> > to any frame regardless of whether we set it or not.
> >> > 2. FDB entries learned via frames applied PVID are registered with
> >> > VID 0 rather than VID value of PVID.
> >> > 3. We can set 0 or 4095 as a PVID that are not allowed in IEEE 802.1Q.
> >> > This leads interoperational problems such as sending frames with VID
> >> > 4095, which is not allowed in IEEE 802.1Q, and treating frames with VID
> >> > 0 as they belong to VLAN 0, which is expected to be handled as they have
> >> > no VID according to IEEE 802.1Q.
> >> >
> >> > Note: 2nd and 3rd problems are potential and not exposed unless 1st problem
> >> > is fixed, because we cannot activate PVID due to it.
> >>
> >> Please work out the issues in patch #2 with Vlad and resubmit this
> >> series.
> >>
> >> Thank you.
> >
> >I'm hovering between whether we should fix the issue by changing vlan 0
> >interface behavior in 8021q module or enabling a bridge port to sending
> >priority-tagged frames, or another better way.
> 
> Take a look at how was it done for bonding - it just goes through the list
> of attached vlan devs, and doesn't care about vlan0 (which can, btw, exist
> technically). I'm not sure if that's what you're looking for, but worth a
> try.
> 
> bond_arp_send_all() might be a good starting point.

Thanks a lot.
I looked over it and took it that bonding driver doesn't send any arp
probe as a priority-tagged frame even if the routing table tells that
the outgoing interface to reach the arp target is bond0.0.

If we seek to take a similar way with bonding, we might be able to
simply ignore vlan 0 interface, or incoming priority-tagged frames.
I'm, however, afraid that it may be undesirable in view of
interoperability.

Anyway, thank you again for your suggestion.
I'm going to think a bit more about the issue and rearrange the patch
set.

Thanks,

Toshiaki Makita

> 
> >
> >If you could comment it, I'd appreciate it :)
> >
> >
> >BTW, I think what is discussed in patch #2 is another problem about
> >handling priority-tags, and it exists without this patch set applied.
> >It looks like that we should prepare another patch set than this to fix
> >that problem.
> >
> >Should I include patches that fix the priority-tags problem in this
> >patch set and resubmit them all together?
> >
> >
> >Thanks,
> >
> >Toshiaki Makita
> >
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH 1/1] isdn: hfcpci_softirq: get func return to suppress compiler warning
From: Antonio Alecrim Jr @ 2013-09-14 17:20 UTC (permalink / raw)
  To: Karsten Keil, Masanari Iida, netdev, linux-kernel; +Cc: Antonio Alecrim Jr

Signed-off-by: Antonio Alecrim Jr <antonio.alecrim@gmail.com>
---
 drivers/isdn/hardware/mISDN/hfcpci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index 7f910c7..3c92780 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2295,8 +2295,8 @@ _hfcpci_softirq(struct device *dev, void *arg)
 static void
 hfcpci_softirq(void *arg)
 {
-	(void) driver_for_each_device(&hfc_driver.driver, NULL, arg,
-				      _hfcpci_softirq);
+	WARN_ON_ONCE(driver_for_each_device(&hfc_driver.driver, NULL, arg,
+				      _hfcpci_softirq) != 0);
 
 	/* if next event would be in the past ... */
 	if ((s32)(hfc_jiffies + tics - jiffies) <= 0)
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH net-next 1/2] bridge: use br_port_get_rtnl within rtnl lock
From: Eric Dumazet @ 2013-09-14 18:46 UTC (permalink / raw)
  To: Hong Zhiguo; +Cc: netdev, davem, vyasevic, Hong Zhiguo
In-Reply-To: <1379169748-767-2-git-send-email-zhiguohong@tencent.com>

On Sat, 2013-09-14 at 22:42 +0800, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
> 
> current br_port_get_rcu is problematic in bridging path
> (NULL deref). Change these calls in netlink path first.
> 
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
>  net/bridge/br_netlink.c | 4 ++--
>  net/bridge/br_private.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

This is not net-next patch, but suitable for net tree, as the following
one fixes a potential panic.

^ permalink raw reply

* Re: [PATCH net-next 2/2] bridge: fix NULL pointer deref of br_port_get_rcu
From: Eric Dumazet @ 2013-09-14 18:47 UTC (permalink / raw)
  To: Hong Zhiguo; +Cc: netdev, davem, vyasevic, Hong Zhiguo
In-Reply-To: <1379169748-767-3-git-send-email-zhiguohong@tencent.com>

On Sat, 2013-09-14 at 22:42 +0800, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
> 
> The NULL deref happens when br_handle_frame is called between these
> 2 lines of del_nbp:
> 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> 	/* --> br_handle_frame is called at this time */
> 	netdev_rx_handler_unregister(dev);
> 
> In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
> without check but br_port_get_rcu(dev) returns NULL if:
> 	!(dev->priv_flags & IFF_BRIDGE_PORT)
> 
> Eric Dumazet pointed out the testing of IFF_BRIDGE_PORT is not necessary
> here since we're in rcu_read_lock and we have synchronize_net() in
> netdev_rx_handler_unregister. So remove the testing of IFF_BRIDGE_PORT
> and by the previous patch, make sure br_port_get_rcu is called in
> bridging code.
> 
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
>  net/bridge/br_private.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

Again this is suitable for net tree.

Thanks !

^ permalink raw reply

* Re: [PATCH] net, mellanox mlx4 Fix compile warnings
From: Or Gerlitz @ 2013-09-14 19:10 UTC (permalink / raw)
  To: Prarit Bhargava, Jack Morgenstein
  Cc: netdev@vger.kernel.org, Doug Ledford, Amir Vadai, Or Gerlitz
In-Reply-To: <1379075438-20240-1-git-send-email-prarit@redhat.com>

On Fri, Sep 13, 2013 at 3:30 PM, Prarit Bhargava <prarit@redhat.com> wrote:
> Fix unitialized variable warnings.

Hi,

I'd like Jack, who is in charge on this code to take a look, added him.

Or.

>
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_CQ_wrapper’:
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2551:16: error: ‘cq’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   atomic_dec(&cq->mtt->ref_count);
>                 ^
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_SRQ_wrapper’:
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2734:17: error: ‘srq’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   atomic_dec(&srq->mtt->ref_count);
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: dledford@redhat.com
> Cc: Amir Vadai <amirv@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlx4/resource_tracker.c  |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> index dd68763..d703838 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> @@ -2563,7 +2563,7 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave,
>  {
>         int err;
>         int cqn = vhcr->in_modifier;
> -       struct res_cq *cq;
> +       struct res_cq *uninitialized_var(cq);
>
>         err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq);
>         if (err)
> @@ -2746,7 +2746,7 @@ int mlx4_HW2SW_SRQ_wrapper(struct mlx4_dev *dev, int slave,
>  {
>         int err;
>         int srqn = vhcr->in_modifier;
> -       struct res_srq *srq;
> +       struct res_srq *uninitialized_var(srq);
>
>         err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED, &srq);
>         if (err)
> --
> 1.7.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] ptp: measure the time offset between PHC and system clock
From: Sergei Shtylyov @ 2013-09-14 19:21 UTC (permalink / raw)
  To: Dong Zhu; +Cc: Richard Cochran, Rob Landley, netdev, linux-doc, linux-kernel
In-Reply-To: <20130914080306.GC23682@zhudong.nay.redhat.com>

Hello.

On 09/14/2013 12:03 PM, Dong Zhu wrote:

> This patch add a method into testptp.c to measure the time offset
> between phc and system clock through the ioctl PTP_SYS_OFFSET.

> Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> ---
>   Documentation/ptp/testptp.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)

> diff --git a/Documentation/ptp/testptp.c b/Documentation/ptp/testptp.c
> index f59ded0..72bb030 100644
> --- a/Documentation/ptp/testptp.c
> +++ b/Documentation/ptp/testptp.c
[...]
> @@ -376,6 +387,31 @@ int main(int argc, char *argv[])
>   		}
>   	}
>
> +	if (offset) {
> +		sysoff = calloc(1, sizeof(*sysoff));
> +		if (!sysoff) {
> +			perror("calloc");
> +			return -1;
> +		}
> +		sysoff->n_samples = n_samples;
> +
> +		if (ioctl(fd, PTP_SYS_OFFSET, sysoff))
> +			perror("PTP_SYS_OFFSET");
> +		else
> +			puts("time offset between PHC and
> +					 system clock request okay");

    Don't break the string constant that way, there'll be all spaces between 
"and" and "system" included in it. Do it like this:

			puts("time offset between PHC and "
			     "system clock request okay");

WBR, Sergei


^ permalink raw reply

* Re: [PATCH 6/7] rtlwifi: Fix smatch warnings in usb.c
From: Sergei Shtylyov @ 2013-09-14 19:44 UTC (permalink / raw)
  To: Larry Finger
  Cc: linville-2XuSBdqkA4R54TAoqtyWWQ,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1379094304-22041-7-git-send-email-Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>

Hello.

On 09/13/2013 09:45 PM, Larry Finger wrote:

> Smatch displays the following:
>    CHECK   drivers/net/wireless/rtlwifi/usb.c
> drivers/net/wireless/rtlwifi/usb.c:458 _rtl_usb_rx_process_agg() warn: assigning (-98) to unsigned variable 'stats.noise'
> drivers/net/wireless/rtlwifi/usb.c:503 _rtl_usb_rx_process_noagg() warn: assigning (-98) to unsigned variable 'stats.noise'
> drivers/net/wireless/rtlwifi/usb.c:596 _rtl_rx_get_padding() info: ignoring unreachable code.
> drivers/net/wireless/rtlwifi/usb.c:596 _rtl_rx_get_padding() info: ignoring unreachable code.

> The negative number to an unsigned quantity is fixed by adding 256 to -98
> to get the equivalent negative number.

> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>   drivers/net/wireless/rtlwifi/usb.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

> diff --git a/drivers/net/wireless/rtlwifi/usb.c b/drivers/net/wireless/rtlwifi/usb.c
> index e56778c..9f3dcb8 100644
> --- a/drivers/net/wireless/rtlwifi/usb.c
> +++ b/drivers/net/wireless/rtlwifi/usb.c
[...]
> @@ -582,12 +582,15 @@ static void _rtl_rx_work(unsigned long param)
>   static unsigned int _rtl_rx_get_padding(struct ieee80211_hdr *hdr,
>   					unsigned int len)
>   {
> +#if NET_IP_ALIGN != 0
>   	unsigned int padding = 0;
> +#endif
>
>   	/* make function no-op when possible */
> -	if (NET_IP_ALIGN == 0 || len < sizeof(*hdr))
> +	if (NET_IP_ALIGN == 0 || len < sizeof(struct ieee80211_hdr))

    Why this collateral and undocumented change? What does it achieve?

>   		return 0;

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/7] rtlwifi: rtl8192de: Fix smatch warnings in rtl8192de/hw.c
From: Sergei Shtylyov @ 2013-09-14 19:59 UTC (permalink / raw)
  To: Larry Finger
  Cc: linville-2XuSBdqkA4R54TAoqtyWWQ,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1379094304-22041-3-git-send-email-Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>

Hello.

On 09/13/2013 09:44 PM, Larry Finger wrote:

> Smatch lists the following:
>    CHECK   drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c:1200 rtl92de_set_qos() info: ignoring unreachable code.
> drivers/net/wireless/rtlwifi/rtl8192de/hw.c:1200 rtl92de_set_qos() info: ignoring unreachable code.

> Dead code is removed.

    It is instead commented out, including non-dead code it seems...

> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>   drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> index 7dd8f6d..c9b0894 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
> @@ -1194,6 +1194,7 @@ void rtl92d_linked_set_reg(struct ieee80211_hw *hw)
>    * mac80211 will send pkt when scan */
>   void rtl92de_set_qos(struct ieee80211_hw *hw, int aci)
>   {
> +/*
>   	struct rtl_priv *rtlpriv = rtl_priv(hw);
>   	rtl92d_dm_init_edca_turbo(hw);
>   	return;

    Shouldn't the comment start here (and *return* removed)? It's also
better to remove the dead code than just to comment it out.

> @@ -1213,6 +1214,7 @@ void rtl92de_set_qos(struct ieee80211_hw *hw, int aci)
>   		RT_ASSERT(false, "invalid aci: %d !\n", aci);
>   		break;
>   	}
> + */
>   }

WBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 6/7] rtlwifi: Fix smatch warnings in usb.c
From: Larry Finger @ 2013-09-14 20:26 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: linville-2XuSBdqkA4R54TAoqtyWWQ,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5234BCB6.6050508-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>

On 09/14/2013 02:44 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 09/13/2013 09:45 PM, Larry Finger wrote:
>
>> Smatch displays the following:
>>    CHECK   drivers/net/wireless/rtlwifi/usb.c
>> drivers/net/wireless/rtlwifi/usb.c:458 _rtl_usb_rx_process_agg() warn:
>> assigning (-98) to unsigned variable 'stats.noise'
>> drivers/net/wireless/rtlwifi/usb.c:503 _rtl_usb_rx_process_noagg() warn:
>> assigning (-98) to unsigned variable 'stats.noise'
>> drivers/net/wireless/rtlwifi/usb.c:596 _rtl_rx_get_padding() info: ignoring
>> unreachable code.
>> drivers/net/wireless/rtlwifi/usb.c:596 _rtl_rx_get_padding() info: ignoring
>> unreachable code.
>
>> The negative number to an unsigned quantity is fixed by adding 256 to -98
>> to get the equivalent negative number.
>
>> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
>> ---
>>   drivers/net/wireless/rtlwifi/usb.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>
>> diff --git a/drivers/net/wireless/rtlwifi/usb.c
>> b/drivers/net/wireless/rtlwifi/usb.c
>> index e56778c..9f3dcb8 100644
>> --- a/drivers/net/wireless/rtlwifi/usb.c
>> +++ b/drivers/net/wireless/rtlwifi/usb.c
> [...]
>> @@ -582,12 +582,15 @@ static void _rtl_rx_work(unsigned long param)
>>   static unsigned int _rtl_rx_get_padding(struct ieee80211_hdr *hdr,
>>                       unsigned int len)
>>   {
>> +#if NET_IP_ALIGN != 0
>>       unsigned int padding = 0;
>> +#endif
>>
>>       /* make function no-op when possible */
>> -    if (NET_IP_ALIGN == 0 || len < sizeof(*hdr))
>> +    if (NET_IP_ALIGN == 0 || len < sizeof(struct ieee80211_hdr))
>
>     Why this collateral and undocumented change? What does it achieve?
>
>>           return 0;

It does not change a thing. Further up the code is "struct ieee80211_hdr *hdr". 
This change was one I tried during the fixes, and I forgot to remove it.

Larry

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/7] rtlwifi: rtl8192de: Fix smatch warnings in rtl8192de/hw.c
From: Larry Finger @ 2013-09-14 20:30 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: linville, linux-wireless, netdev
In-Reply-To: <5234C03A.2070701@cogentembedded.com>

On 09/14/2013 02:59 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 09/13/2013 09:44 PM, Larry Finger wrote:
>
>> Smatch lists the following:
>>    CHECK   drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> drivers/net/wireless/rtlwifi/rtl8192de/hw.c:1200 rtl92de_set_qos() info:
>> ignoring unreachable code.
>> drivers/net/wireless/rtlwifi/rtl8192de/hw.c:1200 rtl92de_set_qos() info:
>> ignoring unreachable code.
>
>> Dead code is removed.
>
>     It is instead commented out, including non-dead code it seems...
>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> ---
>>   drivers/net/wireless/rtlwifi/rtl8192de/hw.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> index 7dd8f6d..c9b0894 100644
>> --- a/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/hw.c
>> @@ -1194,6 +1194,7 @@ void rtl92d_linked_set_reg(struct ieee80211_hw *hw)
>>    * mac80211 will send pkt when scan */
>>   void rtl92de_set_qos(struct ieee80211_hw *hw, int aci)
>>   {
>> +/*
>>       struct rtl_priv *rtlpriv = rtl_priv(hw);
>>       rtl92d_dm_init_edca_turbo(hw);
>>       return;
>
>     Shouldn't the comment start here (and *return* removed)? It's also
> better to remove the dead code than just to comment it out.

That would leave some unused variables.

>> @@ -1213,6 +1214,7 @@ void rtl92de_set_qos(struct ieee80211_hw *hw, int aci)
>>           RT_ASSERT(false, "invalid aci: %d !\n", aci);
>>           break;
>>       }
>> + */
>>   }

I'm not sure what that unreachable code might do, thus I saved it as a comment 
for possible future use. I need to do further evaluation on this fragment, and 
probably consult with the Realtek engineers.

Larry

^ permalink raw reply

* Re: [RFC PATCH 2/4] net: Add lower dev list helpers
From: John Fastabend @ 2013-09-14 20:43 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: stephen, bhutchings, ogerlitz, john.ronciak, netdev,
	shannon.nelson
In-Reply-To: <20130914122756.GA18327@redhat.com>

On 09/14/2013 05:27 AM, Veaceslav Falico wrote:
> On Wed, Sep 11, 2013 at 11:46:49AM -0700, John Fastabend wrote:
>> This patch adds helpers to traverse the lower dev lists, these
>> helpers match the upper dev list implementation.
>>
>> VSI implementers may use these to track a list of connected netdevs.
>> This is easier then having drivers do their own accounting.
>
> Just as a note (as I have quite no idea how ixgbe works) - you are aware
> that the upper/lower lists currently include *all* upper/lower devices, and
> not only the first-connected ones?

Yep, the netif_is_vsi_port() should hopefully catch all the cases.

>
> I've seen that you usually verify it, however not always, just a heads-up -
> sorry if misread.

I'll audit it again, maybe I missed a case.

>
> I'm also currently trying to get the new patchset included - which would
> split the first-tier connected devices from all the 'other' (as in - lower
> of a lower and upper of an upper) devices, so that way, I think, it would
> be easier/faster for you to use it. It also has a ->private field, easily
> accessible, which you could have used instead of/in conjunction with/for
> struct ixgbe_vsi_adapter.

Yeah I saw the patch set I'll use the improved lists once I see the
series gets applied. Although in my case the list walking only occurs in
the configuration path so its not critical.

Also the net device structure already has private space to hang the
device dependent structure off of, so I'm not sure having another
private field helps here.

Thanks,
John

-- 
John Fastabend         Intel Corporation

^ permalink raw reply

* Re: [RFC PATCH 2/4] net: Add lower dev list helpers
From: Veaceslav Falico @ 2013-09-14 21:14 UTC (permalink / raw)
  To: John Fastabend
  Cc: stephen, bhutchings, ogerlitz, john.ronciak, netdev,
	shannon.nelson
In-Reply-To: <5234CA5D.5020603@gmail.com>

On Sat, Sep 14, 2013 at 01:43:09PM -0700, John Fastabend wrote:
>On 09/14/2013 05:27 AM, Veaceslav Falico wrote:
>>On Wed, Sep 11, 2013 at 11:46:49AM -0700, John Fastabend wrote:
>>>This patch adds helpers to traverse the lower dev lists, these
>>>helpers match the upper dev list implementation.
>>>
>>>VSI implementers may use these to track a list of connected netdevs.
>>>This is easier then having drivers do their own accounting.
>>
>>Just as a note (as I have quite no idea how ixgbe works) - you are aware
>>that the upper/lower lists currently include *all* upper/lower devices, and
>>not only the first-connected ones?
>
>Yep, the netif_is_vsi_port() should hopefully catch all the cases.
>
>>
>>I've seen that you usually verify it, however not always, just a heads-up -
>>sorry if misread.
>
>I'll audit it again, maybe I missed a case.

It was about lower devices, and I think I've just really misread it -
indeed they can be only 'your' devices.

>
>>
>>I'm also currently trying to get the new patchset included - which would
>>split the first-tier connected devices from all the 'other' (as in - lower
>>of a lower and upper of an upper) devices, so that way, I think, it would
>>be easier/faster for you to use it. It also has a ->private field, easily
>>accessible, which you could have used instead of/in conjunction with/for
>>struct ixgbe_vsi_adapter.
>
>Yeah I saw the patch set I'll use the improved lists once I see the
>series gets applied. Although in my case the list walking only occurs in
>the configuration path so its not critical.
>
>Also the net device structure already has private space to hang the
>device dependent structure off of, so I'm not sure having another
>private field helps here.

Yep, it should/can be used when linking devices which might already be
using their private space for their own structures, which is not your case,
I suppose.

>
>Thanks,
>John
>
>-- 
>John Fastabend         Intel Corporation

^ permalink raw reply

* [PATCH] bgmac: fix internal switch initialization
From: Rafał Miłecki @ 2013-09-14 22:22 UTC (permalink / raw)
  To: netdev, David S. Miller; +Cc: Hauke Mehrtens, Rafał Miłecki, stable

Some devices (BCM4749, BCM5357, BCM53572) have internal switch that
requires initialization. We already have code for this, but because
of the typo in code it was never working. This resulted in network not
working for some routers and possibility of soft-bricking them.

Use correct bit for switch initialization and fix typo in the define.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/broadcom/bgmac.c |    2 +-
 drivers/net/ethernet/broadcom/bgmac.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index eec0af4..1c6bc96 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -908,7 +908,7 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
 		struct bcma_drv_cc *cc = &bgmac->core->bus->drv_cc;
 		u8 et_swtype = 0;
 		u8 sw_type = BGMAC_CHIPCTL_1_SW_TYPE_EPHY |
-			     BGMAC_CHIPCTL_1_IF_TYPE_RMII;
+			     BGMAC_CHIPCTL_1_IF_TYPE_MII;
 		char buf[2];
 
 		if (bcm47xx_nvram_getenv("et_swtype", buf, 1) > 0) {
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 98d4b5f..12a35cf 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -333,7 +333,7 @@
 
 #define BGMAC_CHIPCTL_1_IF_TYPE_MASK		0x00000030
 #define BGMAC_CHIPCTL_1_IF_TYPE_RMII		0x00000000
-#define BGMAC_CHIPCTL_1_IF_TYPE_MI		0x00000010
+#define BGMAC_CHIPCTL_1_IF_TYPE_MII		0x00000010
 #define BGMAC_CHIPCTL_1_IF_TYPE_RGMII		0x00000020
 #define BGMAC_CHIPCTL_1_SW_TYPE_MASK		0x000000C0
 #define BGMAC_CHIPCTL_1_SW_TYPE_EPHY		0x00000000
-- 
1.7.10.4

^ permalink raw reply related

* Re: [patch 2/4] mISDN: add support for group membership check
From: Ben Hutchings @ 2013-09-14 23:28 UTC (permalink / raw)
  To: akpm, jeffm; +Cc: davem, netdev, isdn4linux, isdn, jslaby, sergei.shtylyov
In-Reply-To: <20130913215202.7D16C31C1BF@corp2gmr1-1.hot.corp.google.com>

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

On Fri, 2013-09-13 at 14:52 -0700, akpm@linux-foundation.org wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> Subject: mISDN: add support for group membership check
> 
> This patch adds a module parameter to allow a group access to the mISDN
> devices.

This doesn't just 'add support' or 'allow a group access'.  It also
changes the default behaviour.

[...]
> --- a/drivers/isdn/mISDN/core.c~misdn-add-support-for-group-membership-check
> +++ a/drivers/isdn/mISDN/core.c
> @@ -21,10 +21,14 @@
>  #include "core.h"
>  
>  static u_int debug;
> +static u_int gid;
> +kgid_t misdn_permitted_gid;
>  
>  MODULE_AUTHOR("Karsten Keil");
>  MODULE_LICENSE("GPL");
>  module_param(debug, uint, S_IRUGO | S_IWUSR);
> +module_param(gid, uint, 0);

So you can't change the privileged gid after loading, or even see what
it is.

[...]
> --- a/drivers/isdn/mISDN/socket.c~misdn-add-support-for-group-membership-check
> +++ a/drivers/isdn/mISDN/socket.c
> @@ -612,6 +612,11 @@ data_sock_create(struct net *net, struct
>  {
>  	struct sock *sk;
>  
> +	if (!capable(CAP_SYS_ADMIN) &&
> +			!gid_eq(misdn_permitted_gid, current_gid()) &&
> +			!in_group_p(misdn_permitted_gid))
> +		return -EPERM;
> +
>  	if (sock->type != SOCK_DGRAM)
>  		return -ESOCKTNOSUPPORT;
>  

I'm pretty sure you could restrict this with LSMs, in a much more
flexible way.

> @@ -694,6 +699,10 @@ base_sock_ioctl(struct socket *sock, uns
>  	case IMSETDEVNAME:
>  	{
>  		struct mISDN_devrename dn;
> +		if (!capable(CAP_SYS_ADMIN) &&
> +				!gid_eq(misdn_permitted_gid, current_gid()) &&
> +				!in_group_p(misdn_permitted_gid))
> +			return -EPERM;
>  		if (copy_from_user(&dn, (void __user *)arg,
>  				   sizeof(dn))) {
>  			err = -EFAULT;

This seems to be the important bit: renaming of devices (if allowed at
all) ought to be limited to CAP_SYS_ADMIN or possibly CAP_NET_ADMIN.
But why should the group that is allowed to use mISDN data sockets also
be allowed to do this?

Ben.

-- 
Ben Hutchings
In a hierarchy, every employee tends to rise to his level of incompetence.

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

^ permalink raw reply

* mvneta: oops in __rcu_read_lock on mirabox
From: Ethan Tuttle @ 2013-09-15  1:05 UTC (permalink / raw)
  To: Thomas Petazzoni, Willy Tarreau, netdev, linux-arm-kernel

When I upgraded my mirabox from 3.11-rc4 to 3.11, I started seeing
oopses while receiving network traffic (see below).  Sending a flood
ping will trigger the oops within a few minutes.

The stack looks similar, but not identical to, the one reported
earlier by Jochen De Smet[1].  In my case the PC is always
__rcu_read_lock.

A git bisect found a878764 "Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net" to be the
first bad commit... interesting, because neither of the merge parents
produce the oops.  I rebased the net changes onto the other merge
parent and bisected that series, which identified 702821f "net: revert
8728c544a9c ("net: dev_pick_tx() fix")" as the first bad commit.
Indeed, reverting 702821f from 3.11 produces a kernel which stands up
to a ping flood for hours.

Each of the times I reproduced this, it was identified as "Unhandled
prefetch abort: unknown 25 (0x409) at 0xc0036ea0", except once when I
got "unknown 16 (0x400)".

I'm assuming this is an mvneta bug that was exposed by 702821f.
That's just a guess, and I don't have the skills to debug this any
further.  In any case, I figured the maintainers would want to know
about it.

Thanks much,

Ethan

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/196332.html

Unhandled prefetch abort: unknown 25 (0x409) at 0xc0036ea0
Internal error: : 409 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.11.0-ARCH-00005-gecca798 #31
task: c074b140 ti: c0740000 task.ti: c0740000
PC is at __rcu_read_lock+0x1c/0x20
LR is at __netif_receive_skb_core+0x80/0x6fc
pc : [<c0036ea0>]    lr : [<c04528d4>]    psr: 60000113
sp : c0741de8  ip : 5232ad87  fp : ef181800
r10: c073ede4  r9 : c07494b8  r8 : ef181800
r7 : 00000000  r6 : 00000001  r5 : ee972b40  r4 : ee972b40
r3 : c074b140  r2 : 00000001  r1 : 00000042  r0 : 0000ffff
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 2e8cc019  DAC: 00000015
Process swapper/0 (pid: 0, stack limit = 0xc0740240)
Stack: (0xc0741de8 to 0xc0742000)
1de0:                   00000000 c0741e28 ee972b40 ee972b40 ef300c00 00000067
1e00: f014f000 ee972b40 ee972b40 ee972b40 ef300c00 00000067 f014f000 00000000
1e20: ef181800 c0455a70 b685321e 13236156 ee972b40 ee972b40 ef300c00 00000067
1e40: f014f000 00000003 ee972b40 c04562a8 00000000 ef181c80 f014fce0 c03bd688
1e60: 00000000 00000000 ef181ccc 00000001 00000001 00000001 c077e190 00000040
1e80: 00000100 00000000 ef181800 ef181c80 ef300c00 00000000 ef181ccc c03bd860
1ea0: 00000001 c076ebf8 c03bd7b0 ef181ccc c1363dc0 00000001 0000012c c1363dc8
1ec0: 00000040 c077d773 c07420c0 c0456018 c074208c 000044fe 0000000c 00000001
1ee0: c0742090 c0740000 0000000a 3f8bdf7c 00000000 00200000 00000101 c0024368
1f00: c077e190 c001bc60 00000000 0000000c 00000003 000044fd c02f91ac 00000017
1f20: 00000000 c0741f78 000003ff c07484c0 561f5811 00000000 00000000 c0024768
1f40: 00000017 c000e6c0 f0002870 c00579e4 c07a4440 c000851c c00579e4 60000013
1f60: ffffffff c0741fac c135f4c0 561f5811 00000000 c00118c0 c13629b0 00000000
1f80: 00000000 00000000 c0740000 c077dd00 c055cc88 c0735450 c135f4c0 561f5811
1fa0: 00000000 00000000 00000002 c0741fc0 c000e980 c00579e4 60000013 ffffffff
1fc0: c0740000 c0711a38 ffffffff ffffffff c0711544 00000000 00000000 c0735450
1fe0: 10c5387d c07484fc c073544c c074c290 00004059 00008074 00000000 00000000
[<c0036ea0>] (__rcu_read_lock+0x1c/0x20) from [<c04528d4>]
(__netif_receive_skb_core+0x80/0x6fc)
[<c04528d4>] (__netif_receive_skb_core+0x80/0x6fc) from [<c0455a70>]
(netif_receive_skb+0x60/0xb8)
[<c0455a70>] (netif_receive_skb+0x60/0xb8) from [<c04562a8>]
(napi_gro_receive+0x48/0x98)
[<c04562a8>] (napi_gro_receive+0x48/0x98) from [<c03bd688>]
(mvneta_rx+0x244/0x36c)
[<c03bd688>] (mvneta_rx+0x244/0x36c) from [<c03bd860>] (mvneta_poll+0xb0/0x15c)
[<c03bd860>] (mvneta_poll+0xb0/0x15c) from [<c0456018>]
(net_rx_action+0x70/0x170)
[<c0456018>] (net_rx_action+0x70/0x170) from [<c0024368>]
(__do_softirq+0xd4/0x1c8)
[<c0024368>] (__do_softirq+0xd4/0x1c8) from [<c0024768>] (irq_exit+0x74/0x88)
[<c0024768>] (irq_exit+0x74/0x88) from [<c000e6c0>] (handle_IRQ+0x68/0x8c)
[<c000e6c0>] (handle_IRQ+0x68/0x8c) from [<c000851c>]
(armada_370_xp_handle_irq+0x44/0xa4)
[<c000851c>] (armada_370_xp_handle_irq+0x44/0xa4) from [<c00118c0>]
(__irq_svc+0x40/0x70)
Exception stack(0xc0741f78 to 0xc0741fc0)
1f60:                                                       c13629b0 00000000
1f80: 00000000 00000000 c0740000 c077dd00 c055cc88 c0735450 c135f4c0 561f5811
1fa0: 00000000 00000000 00000002 c0741fc0 c000e980 c00579e4 60000013 ffffffff
[<c00118c0>] (__irq_svc+0x40/0x70) from [<c00579e4>]
(cpu_startup_entry+0xb0/0x114)
[<c00579e4>] (cpu_startup_entry+0xb0/0x114) from [<c0711a38>]
(start_kernel+0x2c8/0x324)
Code: e593300c e59321b4 e2822001 e58321b4 (e12fff1e)
---[ end trace 8f21018165664a9e ]---
Kernel panic - not syncing: Fatal exception in interrupt

^ permalink raw reply

* Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers
From: Thiago Farina @ 2013-09-15  4:27 UTC (permalink / raw)
  To: Andre Naujoks
  Cc: David S. Miller, Andrew Morton, Steven Rostedt, Rusty Russell,
	Arnd Bergmann, Michael S. Tsirkin, Vladimir Kondratiev,
	Jason Baron, Greg Kroah-Hartman, linux list, linux-can, netdev
In-Reply-To: <1379093833-4949-3-git-send-email-nautsch2@gmail.com>

On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <nautsch2@gmail.com> wrote:
> To be able to use the hex ascii functions in case sensitive environments
> the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
> are introduced.
>
> Signed-off-by: Andre Naujoks <nautsch2@gmail.com>
> ---
>  include/linux/kernel.h | 11 +++++++++++
>  lib/hexdump.c          |  2 ++
>  2 files changed, 13 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 482ad2d..672ddc4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
>         return buf;
>  }
>
> +extern const char hex_asc_upper[];
> +#define hex_asc_upper_lo(x)    hex_asc_upper[((x) & 0x0f)]
> +#define hex_asc_upper_hi(x)    hex_asc_upper[((x) & 0xf0) >> 4]
Does using a macro instead of a real function (static inline)
generates a better code?

--
Thiago Farina

^ permalink raw reply

* Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers
From: Andrew Morton @ 2013-09-15  4:35 UTC (permalink / raw)
  To: Thiago Farina
  Cc: Andre Naujoks, David S. Miller, Steven Rostedt, Rusty Russell,
	Arnd Bergmann, Michael S. Tsirkin, Vladimir Kondratiev,
	Jason Baron, Greg Kroah-Hartman, linux list, linux-can, netdev
In-Reply-To: <CACnwZYdBocYEjASpKCNZ6cfLNpzxeLxaVjLCHwxwsYO+P14tAQ@mail.gmail.com>

On Sun, 15 Sep 2013 01:27:03 -0300 Thiago Farina <tfransosi@gmail.com> wrote:

> On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <nautsch2@gmail.com> wrote:
> > To be able to use the hex ascii functions in case sensitive environments
> > the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
> > are introduced.
> >
> > Signed-off-by: Andre Naujoks <nautsch2@gmail.com>
> > ---
> >  include/linux/kernel.h | 11 +++++++++++
> >  lib/hexdump.c          |  2 ++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 482ad2d..672ddc4 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
> >         return buf;
> >  }
> >
> > +extern const char hex_asc_upper[];
> > +#define hex_asc_upper_lo(x)    hex_asc_upper[((x) & 0x0f)]
> > +#define hex_asc_upper_hi(x)    hex_asc_upper[((x) & 0xf0) >> 4]
> Does using a macro instead of a real function (static inline)
> generates a better code?

Yes, a static inline would be nicer, but these are derived from
hex_asc_lo/hex_asc_hi.  If we change one we should change the other
and that becomes a separate cleanup.  So I think this patch is
OK as-is.

Also, it would make sense to get all the *hex* stuff out of kernel.h
and into a new header file (hexchar.h?).  They're a clean
self-contained thing and kernel.h is rather a dumping ground.


^ permalink raw reply

* Re: [PATCH] net, mellanox mlx4 Fix compile warnings
From: Jack Morgenstein @ 2013-09-15  6:14 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Prarit Bhargava, netdev@vger.kernel.org, Doug Ledford, Amir Vadai,
	Or Gerlitz
In-Reply-To: <CAJZOPZLt8G8MsPxiRNySex=izThR74bXcnWTmWB2wjPys09_ig@mail.gmail.com>

On Sat, 14 Sep 2013 22:10:19 +0300
Or Gerlitz <or.gerlitz@gmail.com> wrote:

> > +       struct res_cq *uninitialized_var(cq);
> >
> >         err = cq_res_start_move_to(dev, slave, cqn,
> > RES_CQ_ALLOCATED, &cq);

I have no objection. However, I don't know if the compiler is being too
clever here or too stupid.  The cq variable is initialized in
cq_res_start_move_to(), but the compiler is ignoring this -- or maybe
it is simply not assuming that cq will in fact be
initialized by the called procedure?

In any event, this change cannot hurt.

-Jack 

^ permalink raw reply

* Re: [PATCH] net, mellanox mlx4 Fix compile warnings
From: ZHAO Gang @ 2013-09-15  6:22 UTC (permalink / raw)
  To: gamerh2o
  Cc: Or Gerlitz, Prarit Bhargava, netdev@vger.kernel.org, Doug Ledford,
	Amir Vadai, Or Gerlitz
In-Reply-To: <20130915091400.49f90bc4@jpm-OptiPlex-GX620>

hell what!

On Sun, Sep 15, 2013 at 09:14:00AM +0300, Jack Morgenstein wrote:
> On Sat, 14 Sep 2013 22:10:19 +0300
> Or Gerlitz <or.gerlitz@gmail.com> wrote:
> 
> > > +       struct res_cq *uninitialized_var(cq);
> > >
> > >         err = cq_res_start_move_to(dev, slave, cqn,
> > > RES_CQ_ALLOCATED, &cq);
> 
> I have no objection. However, I don't know if the compiler is being too
> clever here or too stupid.  The cq variable is initialized in
> cq_res_start_move_to(), but the compiler is ignoring this -- or maybe
> it is simply not assuming that cq will in fact be
> initialized by the called procedure?
> 
> In any event, this change cannot hurt.
> 
> -Jack 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] net, mellanox mlx4 Fix compile warnings
From: ZHAO Gang @ 2013-09-15  6:27 UTC (permalink / raw)
  To: ZHAO Gang
  Cc: Or Gerlitz, Prarit Bhargava, netdev@vger.kernel.org, Doug Ledford,
	Amir Vadai, Or Gerlitz
In-Reply-To: <20130915062125.GA20185@will>

sorry for the inconvenince, i'm just practise the use of mutt, sorry again.

On Sun, Sep 15, 2013 at 2:22 PM, ZHAO Gang <gamerh2o@gmail.com> wrote:
> hell what!
>
> On Sun, Sep 15, 2013 at 09:14:00AM +0300, Jack Morgenstein wrote:
>> On Sat, 14 Sep 2013 22:10:19 +0300
>> Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>
>> > > +       struct res_cq *uninitialized_var(cq);
>> > >
>> > >         err = cq_res_start_move_to(dev, slave, cqn,
>> > > RES_CQ_ALLOCATED, &cq);
>>
>> I have no objection. However, I don't know if the compiler is being too
>> clever here or too stupid.  The cq variable is initialized in
>> cq_res_start_move_to(), but the compiler is ignoring this -- or maybe
>> it is simply not assuming that cq will in fact be
>> initialized by the called procedure?
>>
>> In any event, this change cannot hurt.
>>
>> -Jack
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


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