AW: [PATCH] net: sunrpc: Fix 'snprintf' return value check in 'do_xprt_debugfs'
if xprt->debugfs->d_name.name can be what ever long
it is more clever to use kasprintf()
the some for link (no idea how many xprt als possible)
jm2c
wh
Von: Fedor Tokarev [ftoka...@gmail.com]
Gesendet: Donnerstag, 15. Oktober 2020 15:59
An: bfie...@fieldses.org; chuck.le...@oracle.com; anna.schuma...@netapp.com;
trond.mykleb...@hammerspace.com; da...@davemloft.net; k...@kernel.org
Cc: linux-...@vger.kernel.org; netdev@vger.kernel.org;
linux-ker...@vger.kernel.org; kernel-janit...@vger.kernel.org;
ftoka...@gmail.com
Betreff: [PATCH] net: sunrpc: Fix 'snprintf' return value check in
'do_xprt_debugfs'
'snprintf' returns the number of characters which would have been written
if enough space had been available, excluding the terminating null byte.
Thus, the return value of 'sizeof(buf)' means that the last character
has been dropped.
Signed-off-by: Fedor Tokarev
---
net/sunrpc/debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index fd9bca2..56029e3 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -128,13 +128,13 @@ static int do_xprt_debugfs(struct rpc_clnt *clnt, struct
rpc_xprt *xprt, void *n
return 0;
len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
xprt->debugfs->d_name.name);
- if (len > sizeof(name))
+ if (len >= sizeof(name))
return -1;
if (*nump == 0)
strcpy(link, "xprt");
else {
len = snprintf(link, sizeof(link), "xprt%d", *nump);
- if (len > sizeof(link))
+ if (len >= sizeof(link))
return -1;
}
debugfs_create_symlink(link, clnt->cl_debugfs, name);
--
2.7.4
AW: [PATCH 1/2 v3] net: ethernet: ti: fix some return value check of cpsw_ale_create()
just a notice:
from my casual observation i noticed that most people expect
a function to return NULL on error (as seen here). So i would suggest
to return NULL and (if needed) the error code otherwise.
jm2c,
re
wh
Von: kernel-janitors-ow...@vger.kernel.org
im Auftrag von Wei Yongjun
Gesendet: Mittwoch, 20. Mai 2020 05:41:15
An: netdev@vger.kernel.org; Grygorii Strashko; Christophe JAILLET; Colin Ian
King; Jakub Kicinski
Cc: Wei Yongjun; kernel-janit...@vger.kernel.org; Hulk Robot
Betreff: [PATCH 1/2 v3] net: ethernet: ti: fix some return value check of
cpsw_ale_create()
cpsw_ale_create() can return both NULL and PTR_ERR(), but all of
the caller only check NULL for error handling. This patch convert
it to only return PTR_ERR() in all error cases, and the caller using
IS_ERR() instead of NULL test.
Fixes: 4b41d3436796 ("net: ethernet: ti: cpsw: allow untagged traffic on host
port")
Reported-by: Hulk Robot
Signed-off-by: Wei Yongjun
---
drivers/net/ethernet/ti/cpsw_ale.c| 2 +-
drivers/net/ethernet/ti/cpsw_priv.c | 4 ++--
drivers/net/ethernet/ti/netcp_ethss.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw_ale.c
b/drivers/net/ethernet/ti/cpsw_ale.c
index 0374e6936091..8dc6be11b2ff 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -955,7 +955,7 @@ struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params
*params)
ale = devm_kzalloc(params->dev, sizeof(*ale), GFP_KERNEL);
if (!ale)
- return NULL;
+ return ERR_PTR(-ENOMEM);
ale->p0_untag_vid_mask =
devm_kmalloc_array(params->dev, BITS_TO_LONGS(VLAN_N_VID),
diff --git a/drivers/net/ethernet/ti/cpsw_priv.c
b/drivers/net/ethernet/ti/cpsw_priv.c
index 97a058ca60ac..d0b6c418a870 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c
@@ -490,9 +490,9 @@ int cpsw_init_common(struct cpsw_common *cpsw, void __iomem
*ss_regs,
ale_params.ale_ports= CPSW_ALE_PORTS_NUM;
cpsw->ale = cpsw_ale_create(&ale_params);
- if (!cpsw->ale) {
+ if (IS_ERR(cpsw->ale)) {
dev_err(dev, "error initializing ale engine\n");
- return -ENODEV;
+ return PTR_ERR(cpsw->ale);
}
dma_params.dev = dev;
diff --git a/drivers/net/ethernet/ti/netcp_ethss.c
b/drivers/net/ethernet/ti/netcp_ethss.c
index fb36115e9c51..fdbae734acce 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c
@@ -3704,9 +3704,9 @@ static int gbe_probe(struct netcp_device *netcp_device,
struct device *dev,
ale_params.nu_switch_ale = true;
}
gbe_dev->ale = cpsw_ale_create(&ale_params);
- if (!gbe_dev->ale) {
+ if (IS_ERR(gbe_dev->ale)) {
dev_err(gbe_dev->dev, "error initializing ale engine\n");
- ret = -ENODEV;
+ ret = PTR_ERR(gbe_dev->ale);
goto free_sec_ports;
} else {
dev_dbg(gbe_dev->dev, "Created a gbe ale engine\n");
--
2.25.1
AW: [PATCH] rtlwifi: rtl8192ee: remove redundant for-loop
if someone has same spare time,
this driver need a bit more love ...
SO far i can see in rtl92ee_phy_iq_calibrate:
* IQK_MATRIX_REG_NUM should be used instead 8 hardcoded.
* the for-loop in the beginning is obfuscating that it sets simply
final_candidate
this can be cleaned:
reg_e94 = result[final_candidate][0];
rtlphy->reg_e94 = reg_e94;
reg_e9c = result[final_candidate][1];
rtlphy->reg_e9c = reg_e9c;
only reg_e94, reg_ea4 is used later ?
jm2c,
wh
Von: kernel-janitors-ow...@vger.kernel.org
im Auftrag von Colin King
Gesendet: Freitag, 15. Mai 2020 12:22
An: Kalle Valo; David S . Miller; linux-wirel...@vger.kernel.org;
netdev@vger.kernel.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: [PATCH] rtlwifi: rtl8192ee: remove redundant for-loop
From: Colin Ian King
The for-loop seems to be redundant, the assignments for indexes
0..2 are being over-written by the last index 3 in the loop. Remove
the loop and use index 3 instead.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
---
.../net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
index 6dba576aa81e..bb291b951f4d 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c
@@ -2866,14 +2866,12 @@ void rtl92ee_phy_iq_calibrate(struct ieee80211_hw *hw,
bool b_recovery)
}
}
- for (i = 0; i < 4; i++) {
- reg_e94 = result[i][0];
- reg_e9c = result[i][1];
- reg_ea4 = result[i][2];
- reg_eb4 = result[i][4];
- reg_ebc = result[i][5];
- reg_ec4 = result[i][6];
- }
+ reg_e94 = result[3][0];
+ reg_e9c = result[3][1];
+ reg_ea4 = result[3][2];
+ reg_eb4 = result[3][4];
+ reg_ebc = result[3][5];
+ reg_ec4 = result[3][6];
if (final_candidate != 0xff) {
reg_e94 = result[final_candidate][0];
--
2.25.1
AW: [PATCH] net: dsa: sja1105: fix speed setting for 10 MBPS
IMHO it would be better to use switch case here to improve readability.
switch (bmcr & mask) {
case BMCR_SPEED1000:
speed = SPEED_1000;
break;
case BMCR_SPEED100:
speed = SPEED_100;
break;
case BMCR_SPEED10:
speed = SPEED_10;
break;
default:
speed = SPEED_UNKNOWN
}
jm2c,
wh
btw: an_enabled ? why not !enabled, mich more easy to read
Von: kernel-janitors-ow...@vger.kernel.org
im Auftrag von Colin King
Gesendet: Freitag, 1. Mai 2020 15:43:10
An: Vladimir Oltean; Andrew Lunn; Vivien Didelot; Florian Fainelli; David S .
Miller; Russell King; linux-ker...@vger.kernel.org
Cc: kernel-janit...@vger.kernel.org; netdev@vger.kernel.org
Betreff: [PATCH] net: dsa: sja1105: fix speed setting for 10 MBPS
From: Colin Ian King
The current logic for speed checking will never set the speed to 10 MBPS
because bmcr & BMCR_SPEED10 is always 0 since BMCR_SPEED10 is 0. Also
the erroneous setting where BMCR_SPEED1000 and BMCR_SPEED100 are both
set causes the speed to be 1000 MBS. Fix this by masking bps and checking
for just the expected settings of BMCR_SPEED1000, BMCR_SPEED100 and
BMCR_SPEED10 and defaulting to the unknown speed otherwise.
Addresses-Coverity: ("Logically dead code")
Fixes: ffe10e679cec ("net: dsa: sja1105: Add support for the SGMII port")
Signed-off-by: Colin Ian King
---
drivers/net/dsa/sja1105/sja1105_main.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c
b/drivers/net/dsa/sja1105/sja1105_main.c
index 472f4eb20c49..59a9038cdc4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1600,6 +1600,7 @@ static const char * const sja1105_reset_reasons[] = {
int sja1105_static_config_reload(struct sja1105_private *priv,
enum sja1105_reset_reason reason)
{
+ const int mask = (BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_SPEED10);
struct ptp_system_timestamp ptp_sts_before;
struct ptp_system_timestamp ptp_sts_after;
struct sja1105_mac_config_entry *mac;
@@ -1684,14 +1685,16 @@ int sja1105_static_config_reload(struct sja1105_private
*priv,
sja1105_sgmii_pcs_config(priv, an_enabled, false);
if (!an_enabled) {
- int speed = SPEED_UNKNOWN;
+ int speed;
- if (bmcr & BMCR_SPEED1000)
+ if ((bmcr & mask) == BMCR_SPEED1000)
speed = SPEED_1000;
- else if (bmcr & BMCR_SPEED100)
+ else if ((bmcr & mask) == BMCR_SPEED100)
speed = SPEED_100;
- else if (bmcr & BMCR_SPEED10)
+ else if ((bmcr & mask) == BMCR_SPEED10)
speed = SPEED_10;
+ else
+ speed = SPEED_UNKNOWN;
sja1105_sgmii_pcs_force_speed(priv, speed);
}
--
2.25.1
Re: [PATCH v3] net: ns83820: code cleanup for ns83820_probe_phy()
Am 19.02.2019 10:06, schrieb Mao Wenan:
> This patch is to do code cleanup for ns83820_probe_phy().
> It deletes unused variable 'first' and commented out code.
>
> Signed-off-by: Mao Wenan
> ---
> v2->v3: delte unused variable 'first'; change subject from
> "net: ns83820: drop pointless static qualifier in ns83820_probe_phy()" to
> "net: ns83820: code cleanup for ns83820_probe_phy()".
> drivers/net/ethernet/natsemi/ns83820.c | 18 --
> 1 file changed, 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c
> b/drivers/net/ethernet/natsemi/ns83820.c
> index 958fced4dacf..955d34a6f0d8 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -1869,34 +1869,16 @@ static unsigned ns83820_mii_write_reg(struct ns83820
> *dev, unsigned phy, unsigne
> static void ns83820_probe_phy(struct net_device *ndev)
> {
> struct ns83820 *dev = PRIV(ndev);
> - static int first;
> int i;
> #define MII_PHYIDR1 0x02
> #define MII_PHYIDR2 0x03
>
> -#if 0
> - if (!first) {
> - unsigned tmp;
> - ns83820_mii_read_reg(dev, 1, 0x09);
> - ns83820_mii_write_reg(dev, 1, 0x10, 0x0d3e);
> -
> - tmp = ns83820_mii_read_reg(dev, 1, 0x00);
> - ns83820_mii_write_reg(dev, 1, 0x00, tmp | 0x8000);
> - udelay(1300);
> - ns83820_mii_read_reg(dev, 1, 0x09);
> - }
> -#endif
> - first = 1;
> -
> for (i=1; i<2; i++) {
> int j;
> unsigned a, b;
> a = ns83820_mii_read_reg(dev, i, MII_PHYIDR1);
> b = ns83820_mii_read_reg(dev, i, MII_PHYIDR2);
>
> - //printk("%s: phy %d: 0x%04x 0x%04x\n",
> - // ndev->name, i, a, b);
> -
> for (j=0; j<0x16; j+=4) {
> dprintk("%s: [0x%02x] %04x %04x %04x %04x\n",
> ndev->name, j,
Re: [PATCH v3] net: ns83820: code cleanup for ns83820_probe_phy()
Am 19.02.2019 10:06, schrieb Mao Wenan:
> This patch is to do code cleanup for ns83820_probe_phy().
> It deletes unused variable 'first' and commented out code.
>
> Signed-off-by: Mao Wenan
> ---
> v2->v3: delte unused variable 'first'; change subject from
> "net: ns83820: drop pointless static qualifier in ns83820_probe_phy()" to
> "net: ns83820: code cleanup for ns83820_probe_phy()".
> drivers/net/ethernet/natsemi/ns83820.c | 18 --
> 1 file changed, 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c
> b/drivers/net/ethernet/natsemi/ns83820.c
> index 958fced4dacf..955d34a6f0d8 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -1869,34 +1869,16 @@ static unsigned ns83820_mii_write_reg(struct ns83820
> *dev, unsigned phy, unsigne
> static void ns83820_probe_phy(struct net_device *ndev)
> {
> struct ns83820 *dev = PRIV(ndev);
> - static int first;
> int i;
> #define MII_PHYIDR1 0x02
> #define MII_PHYIDR2 0x03
>
> -#if 0
> - if (!first) {
> - unsigned tmp;
> - ns83820_mii_read_reg(dev, 1, 0x09);
> - ns83820_mii_write_reg(dev, 1, 0x10, 0x0d3e);
> -
> - tmp = ns83820_mii_read_reg(dev, 1, 0x00);
> - ns83820_mii_write_reg(dev, 1, 0x00, tmp | 0x8000);
> - udelay(1300);
> - ns83820_mii_read_reg(dev, 1, 0x09);
> - }
> -#endif
> - first = 1;
> -
> for (i=1; i<2; i++) {
the loop here seems also pointless, so you can eliminate i.
(or did i muss something ?)
just my 2 cents,
re,
wh
> int j;
> unsigned a, b;
> a = ns83820_mii_read_reg(dev, i, MII_PHYIDR1);
> b = ns83820_mii_read_reg(dev, i, MII_PHYIDR2);
>
> - //printk("%s: phy %d: 0x%04x 0x%04x\n",
> - // ndev->name, i, a, b);
> -
> for (j=0; j<0x16; j+=4) {
> dprintk("%s: [0x%02x] %04x %04x %04x %04x\n",
> ndev->name, j,
Re: [PATCH v3 04/21] ppp: exit_net cleanup checks added
Hello Vasily Averin,
just a general hint:
when you send new versions of a patch please document also
what you have changed. Here an example from an other ML:
The problematic code looks like this:
res_seq = res_hdr->xd_hdr.length_sn & TB_XDOMAIN_SN_MASK;
res_seq >>= TB_XDOMAIN_SN_SHIFT;
TB_XDOMAIN_SN_SHIFT is 27, and right shifting a u8 27 bits is always
going to result in zero. The fix is to declare these variables as u32.
Fixes: d1ff70241a27 ("thunderbolt: Add support for XDomain discovery protocol")
Signed-off-by: Dan Carpenter
---
v2: I accidentally sent this through the wrong list, so I'm resending to
netdev. Also Mika asked me to split it up because the Fixes tags
are different for these patches.
please notice the V2. that tell the reader what has changes against
the V1.
re,
wh
Am 06.11.2017 14:23, schrieb Vasily Averin:
> Be sure that lists initialized in net_init hook were return
> to initial state.
>
> Signed-off-by: Vasily Averin
> ---
> drivers/net/ppp/ppp_generic.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index e365866..c0861d1 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -960,6 +960,12 @@ static __net_exit void ppp_exit_net(struct net *net)
> rtnl_unlock();
>
> idr_destroy(&pn->units_idr);
> + WARN_ONCE(!list_empty(&pn->all_channels),
> + "net %x %s: all_channels list is not empty\n",
> + net->ns.inum, __func__);
> + WARN_ONCE(!list_empty(&pn->new_channels),
> + "net %x %s: new_channels list is not empty\n",
> + net->ns.inum, __func__);
> }
>
> static struct pernet_operations ppp_net_ops = {
Re: [PATCH] wan: wanxl: remove redundant assignment to stat
Am 01.11.2017 09:49, schrieb Colin King:
> From: Colin Ian King
>
> stat set to zero and the value is never read, instead stat is
> set again in the do-loop. Hence the setting to zero is redundant
> and can be removed. Cleans up clang warning:
>
> drivers/net/wan/wanxl.c:737:2: warning: Value stored to 'stat'
> is never read
>
> Signed-off-by: Colin Ian King
> ---
> drivers/net/wan/wanxl.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c
> index 0c7317520ed3..d573a57bc301 100644
> --- a/drivers/net/wan/wanxl.c
> +++ b/drivers/net/wan/wanxl.c
> @@ -734,7 +734,6 @@ static int wanxl_pci_init_one(struct pci_dev *pdev,
> return -ENODEV;
> }
>
> - stat = 0;
> timeout = jiffies + 5 * HZ;
> do {
> if ((stat = readl(card->plx + PLX_MAILBOX_5)) != 0)
it is std. practice to have the pattern:
a=b;
if (a == c) ...
maybe that can be done also here.
stat = readl(card->plx + PLX_MAILBOX_5);
if ( stat != 0)
just a hint.
re,
wh
Re: [PATCH 2/2] net: netrom: refactor code in nr_add_node
Am 20.10.2017 18:06, schrieb Gustavo A. R. Silva:
> Hi Walter,
>
> Quoting walter harms :
>
>> Am 19.10.2017 19:27, schrieb Gustavo A. R. Silva:
>>> Code refactoring in order to make the code easier to read and maintain.
>>>
>>> Signed-off-by: Gustavo A. R. Silva
>>> ---
>>> This code was tested by compilation only (GCC 7.2.0 was used).
>>>
>>> net/netrom/nr_route.c | 63
>>> ---
>>> 1 file changed, 20 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
>>> index fc9cadc..1e5165f 100644
>>> --- a/net/netrom/nr_route.c
>>> +++ b/net/netrom/nr_route.c
>>> @@ -80,6 +80,23 @@ static struct nr_neigh
>>> *nr_neigh_get_dev(ax25_address *callsign,
>>>
>>> static void nr_remove_neigh(struct nr_neigh *);
>>>
>>> +/* re-sort the routes in quality order.*/
>>> +static inline void re_sort_routes(struct nr_node *nr_node, int ix_x,
>>> int ix_y)
>>> +{
>>> +struct nr_route nr_route;
>>> +
>>> +if (nr_node->routes[ix_y].quality >
>>> nr_node->routes[ix_x].quality) {
>>> +if (nr_node->which == ix_x)
>>> +nr_node->which = ix_y;
>>> +else if (nr_node->which == ix_y)
>>> +nr_node->which = ix_x;
>>> +
>>> +nr_route = nr_node->routes[ix_x];
>>> +nr_node->routes[ix_x] = nr_node->routes[ix_y];
>>> +nr_node->routes[ix_y] = nr_route;
>>> +}
>>> +}
>>> +
>>
>>
>> Good idea, a bit of nit picking ..
>> does ix_ has a special meaning ? otherwise x,y would be sufficient.
>
> ix typical stands for index, but I think just x and y are fine too.
>
>> From the code below i can see: y=x+1 Perhaps that can be used.
>>
>
> So are you proposing to use two arguments instead of three?
>
> re_sort_routes(nr_node, 0);
>
I am not sure, i would wait a bit and see if what improves readability.
as Kevin Dawson pointed out: this is a sort here.
Maybe there a nice way to do something like that (i do not know):
case 3:
re_sort_routes(nr_node, 1,2)
case 2:
re_sort_routes(nr_node, 0,1)
case 1:
break;
The question is: Is the sorted list needed or simply the maximum ?
NTL is a good thing to chop down the function in smaller digestible
peaces.
re,
wh
>
>> kernel.h has a swap() macro. so you can
>> swap(nr_node->routes[x],nr_node->routes[y]);
>>
>
> Nice, I will use that macro.
>
>> hope that helps,
>
> Definitely. I appreciate your comments.
>
> Thanks!
> --
> Gustavo A. R. Silva
>
>
>
>
>
>
>
Re: [PATCH 2/2] net: netrom: refactor code in nr_add_node
Am 19.10.2017 19:27, schrieb Gustavo A. R. Silva:
> Code refactoring in order to make the code easier to read and maintain.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> This code was tested by compilation only (GCC 7.2.0 was used).
>
> net/netrom/nr_route.c | 63
> ---
> 1 file changed, 20 insertions(+), 43 deletions(-)
>
> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> index fc9cadc..1e5165f 100644
> --- a/net/netrom/nr_route.c
> +++ b/net/netrom/nr_route.c
> @@ -80,6 +80,23 @@ static struct nr_neigh *nr_neigh_get_dev(ax25_address
> *callsign,
>
> static void nr_remove_neigh(struct nr_neigh *);
>
> +/* re-sort the routes in quality order.*/
> +static inline void re_sort_routes(struct nr_node *nr_node, int ix_x, int
> ix_y)
> +{
> + struct nr_route nr_route;
> +
> + if (nr_node->routes[ix_y].quality > nr_node->routes[ix_x].quality) {
> + if (nr_node->which == ix_x)
> + nr_node->which = ix_y;
> + else if (nr_node->which == ix_y)
> + nr_node->which = ix_x;
> +
> + nr_route = nr_node->routes[ix_x];
> + nr_node->routes[ix_x] = nr_node->routes[ix_y];
> + nr_node->routes[ix_y] = nr_route;
> + }
> +}
> +
Good idea, a bit of nit picking ..
does ix_ has a special meaning ? otherwise x,y would be sufficient.
>From the code below i can see: y=x+1 Perhaps that can be used.
kernel.h has a swap() macro. so you can
swap(nr_node->routes[x],nr_node->routes[y]);
hope that helps,
re,
wh
> /*
> * Add a new route to a node, and in the process add the node and the
> * neighbour if it is new.
> @@ -90,7 +107,6 @@ static int __must_check nr_add_node(ax25_address *nr,
> const char *mnemonic,
> {
> struct nr_node *nr_node;
> struct nr_neigh *nr_neigh;
> - struct nr_route nr_route;
> int i, found;
> struct net_device *odev;
>
> @@ -251,50 +267,11 @@ static int __must_check nr_add_node(ax25_address *nr,
> const char *mnemonic,
> /* Now re-sort the routes in quality order */
> switch (nr_node->count) {
> case 3:
> - if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
> - switch (nr_node->which) {
> - case 0:
> - nr_node->which = 1;
> - break;
> - case 1:
> - nr_node->which = 0;
> - break;
> - }
> - nr_route = nr_node->routes[0];
> - nr_node->routes[0] = nr_node->routes[1];
> - nr_node->routes[1] = nr_route;
> - }
> - if (nr_node->routes[2].quality > nr_node->routes[1].quality) {
> - switch (nr_node->which) {
> - case 1: nr_node->which = 2;
> - break;
> -
> - case 2: nr_node->which = 1;
> - break;
> -
> - default:
> - break;
> - }
> - nr_route = nr_node->routes[1];
> - nr_node->routes[1] = nr_node->routes[2];
> - nr_node->routes[2] = nr_route;
> - }
> + re_sort_routes(nr_node, 0, 1);
> + re_sort_routes(nr_node, 1, 2);
> /* fall through */
> case 2:
> - if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
> - switch (nr_node->which) {
> - case 0: nr_node->which = 1;
> - break;
> -
> - case 1: nr_node->which = 0;
> - break;
> -
> - default: break;
> - }
> - nr_route = nr_node->routes[0];
> - nr_node->routes[0] = nr_node->routes[1];
> - nr_node->routes[1] = nr_route;
> - }
> + re_sort_routes(nr_node, 0, 1);
> case 1:
> break;
> }
Re: [PATCH] netfilter: fix indent on in statements
Am 15.08.2017 08:50, schrieb Colin King:
> From: Colin Ian King
>
> The returns on some if statements are not indented correctly,
> add in the missing tab.
>
> Signed-off-by: Colin Ian King
> ---
> net/bridge/netfilter/ebt_ip.c | 4 ++--
> net/bridge/netfilter/ebt_ip6.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebt_ip.c b/net/bridge/netfilter/ebt_ip.c
> index d06968bdf5ec..2b46c50abce0 100644
> --- a/net/bridge/netfilter/ebt_ip.c
> +++ b/net/bridge/netfilter/ebt_ip.c
> @@ -64,14 +64,14 @@ ebt_ip_mt(const struct sk_buff *skb, struct
> xt_action_param *par)
> if (NF_INVF(info, EBT_IP_DPORT,
> dst < info->dport[0] ||
> dst > info->dport[1]))
> - return false;
> + return false;
This is hard to read, perhaps it gets better when the result is stored in a
tmp-var.
something like:
int isbetween=dst < info->dport[0] ||dst > info->dport[1] ;
int state=NF_INVF(info, EBT_IP_DPORT, isbetween );
if ( state )
return false;
just my 2 cents,
re,
wh
> }
> if (info->bitmask & EBT_IP_SPORT) {
> u32 src = ntohs(pptr->src);
> if (NF_INVF(info, EBT_IP_SPORT,
> src < info->sport[0] ||
> src > info->sport[1]))
> - return false;
> + return false;
> }
> }
> return true;
> diff --git a/net/bridge/netfilter/ebt_ip6.c b/net/bridge/netfilter/ebt_ip6.c
> index 4617491be41e..2a5a52a53ec4 100644
> --- a/net/bridge/netfilter/ebt_ip6.c
> +++ b/net/bridge/netfilter/ebt_ip6.c
> @@ -89,7 +89,7 @@ ebt_ip6_mt(const struct sk_buff *skb, struct
> xt_action_param *par)
> if (NF_INVF(info, EBT_IP6_SPORT,
> src < info->sport[0] ||
> src > info->sport[1]))
> - return false;
> + return false;
> }
> if ((info->bitmask & EBT_IP6_ICMP6) &&
> NF_INVF(info, EBT_IP6_ICMP6,
Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
Am 17.05.2017 15:42, schrieb Firo Yang:
> On Wed, May 17, 2017 at 02:59:39PM +0200, walter harms wrote:
>>
>>
>> Am 17.05.2017 14:35, schrieb Firo Yang:
>>> The divisor s->par.bitrate will always be 0 until initialized by
>>> ndo_open() and hdlcdrv_open().
>>>
>>> In order to fix this divide zero error, check whether the netdevice
>>> was opened by ndo_open() before performing divide.
>>>
>>> Reported-by: Dmitry Vyukov
>>> Signed-off-by: Firo Yang
>>> ---
>>> drivers/net/hamradio/hdlcdrv.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/hamradio/hdlcdrv.c b/drivers/net/hamradio/hdlcdrv.c
>>> index 8c3633c..3c783fd 100644
>>> --- a/drivers/net/hamradio/hdlcdrv.c
>>> +++ b/drivers/net/hamradio/hdlcdrv.c
>>> @@ -574,7 +574,7 @@ static int hdlcdrv_ioctl(struct net_device *dev, struct
>>> ifreq *ifr, int cmd)
>>> break;
>>>
>>> case HDLCDRVCTL_CALIBRATE:
>>> - if(!capable(CAP_SYS_RAWIO))
>>> + if (!capable(CAP_SYS_RAWIO) || !netif_running(dev))
>>> return -EPERM;
>>> if (bi.data.calibrate > INT_MAX / s->par.bitrate)
>>> return -EINVAL;
>>
>> I would still check for s->par.bitrate > 0 later changes may affect the
>> setting of it
>> and it is much more obvious.
>
> I think 0 is not valid value for bitrate, so we should check it in
> other places, like what ser12_open() did:
> 429 if (bc->baud < 300 || bc->baud > 4800) {
> 430 printk(KERN_INFO "baycom_ser_fdx: invalid baudrate "
> 431 "(300...4800)\n");
> 432 return -EINVAL;
> 433 }
> ...
> 440 bc->hdrv.par.bitrate = bc->baud;
I do not want to say you change is not valid but i have learned that it is
better to
have an obvious check that to rely on hidden knowledge.
>
>>
>> Also perhaps !netif_running(dev) should better return ENODEV.
>
> However, the 'dev' truly exists in this circumstance.
>
yes and i do not feel good with that but "no permission" will lead
any enduser into a search for user rights.
re,
wh
> Thanks,
> Firo
>
>>
>>
>> just my 2 cents,
>> re,
>> wh
>>
Re: [PATCH] hdlcdrv: fix divide error bug if bitrate is 0
Am 17.05.2017 14:35, schrieb Firo Yang: > The divisor s->par.bitrate will always be 0 until initialized by > ndo_open() and hdlcdrv_open(). > > In order to fix this divide zero error, check whether the netdevice > was opened by ndo_open() before performing divide. > > Reported-by: Dmitry Vyukov > Signed-off-by: Firo Yang > --- > drivers/net/hamradio/hdlcdrv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/hamradio/hdlcdrv.c b/drivers/net/hamradio/hdlcdrv.c > index 8c3633c..3c783fd 100644 > --- a/drivers/net/hamradio/hdlcdrv.c > +++ b/drivers/net/hamradio/hdlcdrv.c > @@ -574,7 +574,7 @@ static int hdlcdrv_ioctl(struct net_device *dev, struct > ifreq *ifr, int cmd) > break; > > case HDLCDRVCTL_CALIBRATE: > - if(!capable(CAP_SYS_RAWIO)) > + if (!capable(CAP_SYS_RAWIO) || !netif_running(dev)) > return -EPERM; > if (bi.data.calibrate > INT_MAX / s->par.bitrate) > return -EINVAL; I would still check for s->par.bitrate > 0 later changes may affect the setting of it and it is much more obvious. Also perhaps !netif_running(dev) should better return ENODEV. just my 2 cents, re, wh
Re: [PATCH] net/sched: remove redundant null check on head
Am 03.05.2017 15:50, schrieb Colin King:
> From: Colin Ian King
>
> head is previously null checked and so the 2nd null check on head
> is redundant and therefore can be removed.
>
> Detected by CoverityScan, CID#1399505 ("Logically dead code")
>
> Signed-off-by: Colin Ian King
> ---
> net/sched/cls_matchall.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 2efb36c08f2a..dee469fed967 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -203,8 +203,7 @@ static int mall_change(struct net *net, struct sk_buff
> *in_skb,
>
> *arg = (unsigned long) head;
> rcu_assign_pointer(tp->root, new);
> - if (head)
> - call_rcu(&head->rcu, mall_destroy_rcu);
> + call_rcu(&head->rcu, mall_destroy_rcu);
> return 0;
>
> err_replace_hw_filter:
if someone cares .. the err=0 in the the line above the patch is also useless
because err is never used again. Merging both if will save 1 indent level.
err = mall_replace_hw_filter(tp, new, (unsigned long) new);
if (err && tc_skip_sw(flags) )
goto errout;
just my impression,
re
wh
Re: [PATCH] Add checks for kmalloc allocation failures
Am 29.03.2017 17:54, schrieb Colin King:
> From: Colin Ian King
>
> Ensure we don't end up with a null pointer dereferences by checking
> for for allocation failures. Allocate by sizeof(*ptr) rather than
> the type to fix checkpack warnings. Also merge multiple lines into
> one line for the kmalloc call.
>
> Detected by CoverityScan, CID#1422435 ("Dereference null return value")
>
> Signed-off-by: Colin Ian King
> ---
> drivers/net/ieee802154/ca8210.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 53fa87bfede0..25fd3b04b3c0 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -634,6 +634,8 @@ static int ca8210_test_int_driver_write(
> dev_dbg(&priv->spi->dev, "%#03x\n", buf[i]);
>
> fifo_buffer = kmalloc(len, GFP_KERNEL);
> + if (!fifo_buffer)
> + return -ENOMEM;
> memcpy(fifo_buffer, buf, len);
perhaps kmemdup() ist the way to go ?
by replace kamlloc()+memcpy
re,
wh
> kfifo_in(&test->up_fifo, &fifo_buffer, 4);
> wake_up_interruptible(&priv->test.readq);
> @@ -759,10 +761,10 @@ static void ca8210_rx_done(struct cas_control *cas_ctl)
> &priv->spi->dev,
> "Resetting MAC...\n");
>
> - mlme_reset_wpc = kmalloc(
> - sizeof(struct work_priv_container),
> - GFP_KERNEL
> - );
> + mlme_reset_wpc = kmalloc(sizeof(*mlme_reset_wpc),
> + GFP_KERNEL);
> + if (!mlme_reset_wpc)
> + goto finish;
> INIT_WORK(
> &mlme_reset_wpc->work,
> ca8210_mlme_reset_worker
> @@ -925,10 +927,10 @@ static int ca8210_spi_transfer(
>
> dev_dbg(&spi->dev, "ca8210_spi_transfer called\n");
>
> - cas_ctl = kmalloc(
> - sizeof(struct cas_control),
> - GFP_ATOMIC
> - );
> + cas_ctl = kmalloc(sizeof(*cas_ctl), GFP_ATOMIC);
> + if (!cas_ctl)
> + return -ENOMEM;
> +
> cas_ctl->priv = priv;
> memset(cas_ctl->tx_buf, SPI_IDLE, CA8210_SPI_BUF_SIZE);
> memset(cas_ctl->tx_in_buf, SPI_IDLE, CA8210_SPI_BUF_SIZE);
Re: [PATCH] VSOCK: remove unnecessary ternary operator on return value
Am 28.03.2017 17:54, schrieb Colin King:
> From: Colin Ian King
>
> Rather than assign the positive errno values to ret and then
> checking if it is positive and flip the sign, just set ret to
> be the -ve errno value.
>
> Detected by CoverityScan, CID#986649 ("Logically Dead Code")
>
> Signed-off-by: Colin Ian King
> ---
> net/vmw_vsock/vmci_transport.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 4be4fbbc0b50..a68cd6b0fb72 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -100,27 +100,27 @@ static s32 vmci_transport_error_to_vsock_error(s32
> vmci_error)
>
> switch (vmci_error) {
> case VMCI_ERROR_NO_MEM:
> - err = ENOMEM;
> + err = -ENOMEM;
> break;
> case VMCI_ERROR_DUPLICATE_ENTRY:
> case VMCI_ERROR_ALREADY_EXISTS:
> - err = EADDRINUSE;
> + err = -EADDRINUSE;
> break;
> case VMCI_ERROR_NO_ACCESS:
> - err = EPERM;
> + err = -EPERM;
> break;
> case VMCI_ERROR_NO_RESOURCES:
> - err = ENOBUFS;
> + err = -ENOBUFS;
> break;
> case VMCI_ERROR_INVALID_RESOURCE:
> - err = EHOSTUNREACH;
> + err = -EHOSTUNREACH;
> break;
> case VMCI_ERROR_INVALID_ARGS:
> default:
> - err = EINVAL;
> + err = -EINVAL;
> }
>
> - return err > 0 ? -err : err;
> + return err;
> }
>
> static u32 vmci_transport_peer_rid(u32 peer_cid)
yes, but because there is nothing to do you could return directly
and forget about err.
note: why is there a MCI_ERROR_NO_MEM when you can map this to ENOMEM ?
re,
wh
Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
Am 13.02.2017 14:03, schrieb Timur Tabi: > walter harms wrote: >> We have a function where the argument is ignored and the rest is const ? >> >> emac_ethtool_get_regs_len seems the only user. So it would be fairly >> easy to >> move that into that function. >> >> @maintainer: >> Is there a deeper logic behind this ? > > I don't understand the question. The question is: why is a simple calculation const*const separated into a function ? re, wh
Re: [patch net-next] net: qcom/emac: fix a sizeof() typo
Am 13.02.2017 12:00, schrieb Dan Carpenter:
> We had intended to say "sizeof(u32)" but the "u" is missing.
> Fortunately, sizeof(32) is also 4, so the original code still works.
>
> Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading
> hardware registers")
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> index 0d9945fb79be..bbe24639aa5a 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> @@ -227,7 +227,7 @@ static void emac_get_regs(struct net_device *netdev,
>
> static int emac_get_regs_len(struct net_device *netdev)
> {
> - return EMAC_MAX_REG_SIZE * sizeof(32);
> + return EMAC_MAX_REG_SIZE * sizeof(u32);
> }
>
We have a function where the argument is ignored and the rest is const ?
emac_ethtool_get_regs_len seems the only user. So it would be fairly easy to
move that into that function.
@maintainer:
Is there a deeper logic behind this ?
re,
wh
> static const struct ethtool_ops emac_ethtool_ops = {
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Re: [PATCH] NET: mkiss/6pack: Fix SIOCSIFENCAP ioctl
Am 11.02.2017 00:01, schrieb Ralf Baechle:
> When looking at Thomas' mkiss fix 7ba1b6890387 ("NET: mkiss: Fix panic")
> I noticed that the mkiss SIOCSIFENCAPS ioctl was also doing a slightly
> strange assignment
>
>dev->hard_header_len = AX25_KISS_HEADER_LEN +
> AX25_MAX_HEADER_LEN + 3;
>
> AX25_MAX_HEADER_LEN already accounts for the KISS byte so adding
> AX25_KISS_HEADER_LEN is a double allocation nor does the "+ 3" seem to
> be necessary. So this can be simplified to
>
>dev->hard_header_len = AX25_MAX_HEADER_LEN
>
> which after the preceeding fix is a redundant assignment of what
> ax_setup has already assigned so delete the line. The assignments
> to dev->addr_len and dev->type are similarly redundant.
>
> The SIOCSIFENCAP argument was never checked for validity. Check that
> it is 4 and return -EINVAL if not. The magic constant 4 dates back to
> the days when KISS was handled by the SLIP driver where it had the
> symbol name SL_MODE_AX25.
>
> Since however mkiss.c only supports a single encapsulation mode there
> is no point in storing it in struct mkiss so delete all that.
>
> Note that while useless we can't delete the SIOCSIFENCAP ioctl as
> kissattach(8) is still using it and without mkiss issuing a
> SIOCSIFENCAP ioctl an older kernel that does not have Thomas' mkiss fix
> would still panic on attempt to transmit via mkiss.
>
> 6pack was suffering from the same issue except there SIOCGIFENCAP was
> return 0 for the encapsulation while the spattach utility was passing 4
> for the mode, so the mode check added for 6pack is a bit more lenient
> allow the values 0 and 4 to be set. That way we retain the option
> to set different encapsulation modes for future extensions.
>
> Signed-off-by: Ralf Baechle
>
> drivers/net/hamradio/6pack.c | 10 --
> drivers/net/hamradio/mkiss.c | 10 --
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
> index 470b3dc..d949b9f 100644
> --- a/drivers/net/hamradio/6pack.c
> +++ b/drivers/net/hamradio/6pack.c
> @@ -104,7 +104,6 @@ struct sixpack {
> int buffsize; /* Max buffers sizes */
>
> unsigned long flags; /* Flag values/ mode etc */
> - unsigned char mode; /* 6pack mode */
>
> /* 6pack stuff */
> unsigned char tx_delay;
> @@ -723,11 +722,10 @@ static int sixpack_ioctl(struct tty_struct *tty, struct
> file *file,
> break;
> }
>
> - sp->mode = tmp;
> - dev->addr_len= AX25_ADDR_LEN;
> - dev->hard_header_len = AX25_KISS_HEADER_LEN +
> -AX25_MAX_HEADER_LEN + 3;
> - dev->type= ARPHRD_AX25;
> + if (tmp != 0 && tmp != 4) {
> + err = -EINVAL;
> + break;
> + }
>
What is about a comment like:
/*
The magic constant 4 dates back to the days when KISS was handled by the SLIP
driver where it had the
symbol name SL_MODE_AX25.
*/
just not to confuse future readers ..
re,
wh
> err = 0;
> break;
> diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
> index 1dfe230..cdaf819 100644
> --- a/drivers/net/hamradio/mkiss.c
> +++ b/drivers/net/hamradio/mkiss.c
> @@ -71,7 +71,6 @@ struct mkiss {
> #define AXF_KEEPTEST 3 /* Keepalive test flag */
> #define AXF_OUTWAIT 4 /* is outpacket was flag*/
>
> - int mode;
> int crcmode;/* MW: for FlexNet, SMACK etc. */
> int crcauto;/* CRC auto mode */
>
> @@ -841,11 +840,10 @@ static int mkiss_ioctl(struct tty_struct *tty, struct
> file *file,
> break;
> }
>
> - ax->mode = tmp;
> - dev->addr_len= AX25_ADDR_LEN;
> - dev->hard_header_len = AX25_KISS_HEADER_LEN +
> -AX25_MAX_HEADER_LEN + 3;
> - dev->type= ARPHRD_AX25;
> + if (tmp != 4) {
> + err = -EINVAL;
> + break;
> + }
>
> err = 0;
> break;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mlx5/core: Use memdup_user() rather than duplicating its implementation
Am 20.08.2016 08:01, schrieb SF Markus Elfring:
> From: Markus Elfring
> Date: Sat, 20 Aug 2016 07:50:09 +0200
>
> * Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
>
> This issue was detected by using the Coccinelle software.
>
> * Return directly if this copy operation failed.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 17 +++--
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 6388bc0..bb89f04 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -1132,7 +1132,6 @@ static ssize_t data_write(struct file *filp, const char
> __user *buf,
> struct mlx5_core_dev *dev = filp->private_data;
> struct mlx5_cmd_debug *dbg = &dev->cmd.dbg;
> void *ptr;
> - int err;
>
> if (*pos != 0)
> return -EINVAL;
> @@ -1140,25 +1139,15 @@ static ssize_t data_write(struct file *filp, const
> char __user *buf,
> kfree(dbg->in_msg);
> dbg->in_msg = NULL;
> dbg->inlen = 0;
> -
> - ptr = kzalloc(count, GFP_KERNEL);
> - if (!ptr)
> - return -ENOMEM;
> -
> - if (copy_from_user(ptr, buf, count)) {
> - err = -EFAULT;
> - goto out;
> - }
> + ptr = memdup_user(buf, count);
> + if (IS_ERR(ptr))
> + return PTR_ERR(ptr);
> dbg->in_msg = ptr;
> dbg->inlen = count;
>
> *pos = count;
>
maybe i am missing something here but why do you need ptr ?
The use of count looks even more confusing it is stored in
dbg->inlen, *pos and is returned.
is that realy needed ?
re,
wh
> return count;
> -
> -out:
> - kfree(ptr);
> - return err;
> }
>
> static ssize_t data_read(struct file *filp, char __user *buf, size_t count,
Re: [patch -next] wan/fsl_ucc_hdlc: info leak in uhdlc_ioctl()
Am 14.07.2016 12:34, schrieb Dan Carpenter:
> There is a 2 byte struct whole after line.loopback so we need to clear
> that out to avoid disclosing stack information.
>
> Fixes: c19b6d246a35 ('drivers/net: support hdlc function for QE-UCC')
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 19174ac..7608561 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -635,6 +635,7 @@ static int uhdlc_ioctl(struct net_device *dev, struct
> ifreq *ifr, int cmd)
> ifr->ifr_settings.size = size; /* data size wanted */
> return -ENOBUFS;
> }
> + memset(&line, 0, sizeof(line));
> line.clock_type = priv->clocking;
> line.clock_rate = 0;
> line.loopback = 0;
In this case
line.clock_rate = 0;
line.loopback = 0;
are not need any more and can be removed
except like them to have for documentation or so.
re,
wh
Re: Inconsistent use of size argument in kzalloc and memcpy in 'drivers/net/ethernet/toshiba/ps3_gelic_wireless.c'
this is a case for kmemdup(). target->hwinfo=kmemdup(scan_info,be16_to_cpu(scan_info->size), GFP_KERNEL); re, wh Am 11.04.2016 12:00, schrieb Christophe JAILLET: > Hi, > > while looking at potential clean-up, I ended on the following code which > looks spurious to me. > > We allocate 'be16_to_cpu(scan_info->size)' bytes, but then copy > 'scan_info->size'. > This is not consistent. > > > I don't know which one is the correct one. > > > CJ > > --- drivers/net/ethernet/toshiba/ps3_gelic_wireless.c > +++ /tmp/cocci-output-24201-0dddbd-ps3_gelic_wireless.c > @@ -1616,13 +1616,10 @@ static void gelic_wl_scan_complete_event > target->valid = 1; > target->eurus_index = i; > kfree(target->hwinfo); > -target->hwinfo = kzalloc(be16_to_cpu(scan_info->size), > - GFP_KERNEL); > if (!target->hwinfo) > continue; > > /* copy hw scan info */ > -memcpy(target->hwinfo, scan_info, scan_info->size); > target->essid_len = strnlen(scan_info->essid, > sizeof(scan_info->essid)); > target->rate_len = 0; > > -- > To unsubscribe from this list: send the line "unsubscribe > kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [RFC PATCH 6/6] ppp: add rtnetlink device creation support
Am 05.04.2016 23:22, schrieb Guillaume Nault:
> On Tue, Apr 05, 2016 at 07:18:14PM +0200, walter harms wrote:
>>
>>
>> Am 05.04.2016 02:56, schrieb Guillaume Nault:
>>> @@ -1043,12 +1048,39 @@ static int ppp_dev_configure(struct net *src_net,
>>> struct net_device *dev,
>>> const struct ppp_config *conf)
>>> {
>>> struct ppp *ppp = netdev_priv(dev);
>>> + struct file *file;
>>> int indx;
>>> + int err;
>>> +
>>> + if (conf->fd < 0) {
>>> + file = conf->file;
>>> + if (!file) {
>>> + err = -EBADF;
>>> + goto out;
>>
>> why not just return -EBADF;
>>
>>> + }
>>> + } else {
>>> + file = fget(conf->fd);
>>> + if (!file) {
>>> + err = -EBADF;
>>> + goto out;
>>
>> why not just return -EBADF;
>>
> Just because the 'out' label is declared anyway and because this
> centralises the return point. But I agree returning -EBADF directly
> could be more readable. I don't have strong opinion.
in this special case i would go for readable. People tend to miss these
if
if
if
constructs.
NTL its up to you.
re,
wh
Re: [RFC PATCH 6/6] ppp: add rtnetlink device creation support
Am 05.04.2016 02:56, schrieb Guillaume Nault:
> Define PPP device handlers for use with rtnetlink.
> The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
> contains the file descriptor of the associated /dev/ppp instance (the
> file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
> the ioctl-based API). The PPP device is removed when this file
> descriptor is released (same behaviour as with ioctl based PPP
> devices).
>
> PPP devices created with the rtnetlink API behave like the ones created
> with ioctl(PPPIOCNEWUNIT). In particular existing ioctls work the same
> way, no matter how the PPP device was created.
>
> However, there are a few differences between rtnl and ioctl based PPP
> devices. Rtnl based PPP devices can be removed with RTM_DELLINK
> messages (e.g. with "ip link del"), while the ones created with
> ioctl(PPPIOCNEWUNIT) can't.
> The interface name is also built differently: the number following the
> "ppp" prefix corresponds to the PPP unit number for ioctl based
> devices, while it is just an unrelated incrementing index for rtnl
> ones.
>
> Signed-off-by: Guillaume Nault
> ---
> drivers/net/ppp/ppp_generic.c | 143
> +-
> include/uapi/linux/if_link.h | 8 +++
> 2 files changed, 136 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 516f8dc..ae40368 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -46,6 +46,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -185,7 +186,9 @@ struct channel {
>
> struct ppp_config {
> struct file *file;
> + s32 fd;
> s32 unit;
> + bool ifname_is_set;
> };
>
> /*
> @@ -286,6 +289,7 @@ static int unit_get(struct idr *p, void *ptr);
> static int unit_set(struct idr *p, void *ptr, int n);
> static void unit_put(struct idr *p, int n);
> static void *unit_find(struct idr *p, int n);
> +static void ppp_setup(struct net_device *dev);
>
> static const struct net_device_ops ppp_netdev_ops;
>
> @@ -989,7 +993,7 @@ static struct pernet_operations ppp_net_ops = {
> .size = sizeof(struct ppp_net),
> };
>
> -static int ppp_unit_register(struct ppp *ppp, int unit)
> +static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
> {
> struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
> int ret;
> @@ -1019,7 +1023,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
> }
> ppp->file.index = ret;
>
> - snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
> + if (!ifname_is_set)
> + snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
>
> ret = register_netdevice(ppp->dev);
> if (ret < 0)
> @@ -1043,12 +1048,39 @@ static int ppp_dev_configure(struct net *src_net,
> struct net_device *dev,
>const struct ppp_config *conf)
> {
> struct ppp *ppp = netdev_priv(dev);
> + struct file *file;
> int indx;
> + int err;
> +
> + if (conf->fd < 0) {
> + file = conf->file;
> + if (!file) {
> + err = -EBADF;
> + goto out;
why not just return -EBADF;
> + }
> + } else {
> + file = fget(conf->fd);
> + if (!file) {
> + err = -EBADF;
> + goto out;
why not just return -EBADF;
> + }
just my 2 cents,
re,
wh
> +
> + if (file->f_op != &ppp_device_fops) {
> + err = -EBADF;
> + goto out;
> + }
> + }
> +
> + mutex_lock(&ppp_mutex);
> + if (file->private_data) {
> + err = -ENOTTY;
> + goto out_mutex;
> + }
>
> ppp->dev = dev;
> ppp->mru = PPP_MRU;
> ppp->ppp_net = src_net;
> - ppp->owner = conf->file;
> + ppp->owner = file;
>
> init_ppp_file(&ppp->file, INTERFACE);
> ppp->file.hdrlen = PPP_HDRLEN - 2; /* don't count proto bytes */
> @@ -1067,9 +1099,88 @@ static int ppp_dev_configure(struct net *src_net,
> struct net_device *dev,
> ppp->active_filter = NULL;
> #endif /* CONFIG_PPP_FILTER */
>
> - return ppp_unit_register(ppp, conf->unit);
> + err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
> + if (err < 0)
> + goto out_mutex;
> +
> + file->private_data = &ppp->file;
> +
> +out_mutex:
> + mutex_unlock(&ppp_mutex);
> +out:
> + if (conf->fd >= 0 && file)
> + fput(file);
> +
> + return err;
> }
>
> +static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
> + [IFLA_PPP_DEV_FD] = { .type = NLA_S32 },
> +};
> +
> +static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
> +{
> + if (!data)
> + return -EINVAL;
> +
> + if (!data[IFLA_PPP_D
Re: [PATCH net 1/3] net: validate variable length ll headers
Am 04.03.2016 21:44, schrieb Willem de Bruijn:
> From: Willem de Bruijn
>
> Netdevice parameter hard_header_len is variously interpreted both as
> an upper and lower bound on link layer header length. The field is
> used as upper bound when reserving room at allocation, as lower bound
> when validating user input in PF_PACKET.
>
> Clarify the definition to be maximum header length. For validation
> of untrusted headers, add an optional validate member to header_ops.
>
> Allow bypassing of validation by passing CAP_SYS_RAWIO, for instance
> for deliberate testing of corrupt input. In this case, pad trailing
> bytes, as some device drivers expect completely initialized headers.
>
> See also http://comments.gmane.org/gmane.linux.network/401064
>
> Signed-off-by: Willem de Bruijn
> ---
> include/linux/netdevice.h | 22 --
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5440b7b..6d1d8f4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -267,6 +267,7 @@ struct header_ops {
> void(*cache_update)(struct hh_cache *hh,
> const struct net_device *dev,
> const unsigned char *haddr);
> + bool(*validate)(const char *ll_header, unsigned int len);
> };
>
> /* These flag bits are private to the generic network queueing
> @@ -1420,8 +1421,7 @@ enum netdev_priv_flags {
> * @dma: DMA channel
> * @mtu: Interface MTU value
> * @type: Interface hardware type
> - * @hard_header_len: Hardware header length, which means that this is the
> - * minimum size of a packet.
> + * @hard_header_len: Maximum hardware header length.
> *
> * @needed_headroom: Extra headroom the hardware may need, but not in all
> * cases can this be guaranteed
> @@ -2627,6 +2627,24 @@ static inline int dev_parse_header(const struct
> sk_buff *skb,
> return dev->header_ops->parse(skb, haddr);
> }
>
> +/* ll_header must have at least hard_header_len allocated */
> +static inline bool dev_validate_header(const struct net_device *dev,
> +char *ll_header, int len)
> +{
> + if (likely(len >= dev->hard_header_len))
> + return true;
> +
> + if (capable(CAP_SYS_RAWIO)) {
> + memset(ll_header + len, 0, dev->hard_header_len - len);
> + return true;
> + }
> +
> + if (dev->header_ops && dev->header_ops->validate)
> + return dev->header_ops->validate(ll_header, len);
> +
> + return false;
> +}
> +
you could use
real_len=dev->hard_header_len-len;
if (real_len < 0)
...
if (capable(CAP_SYS_RAWIO))
memset(ll_header + len, 0,real_len);
..
IMHO that makes the code more clear.
re,
wh
> typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr,
> int len);
> int register_gifconf(unsigned int family, gifconf_func_t *gifconf);
> static inline int unregister_gifconf(unsigned int family)
Re: [PATCH 2/2] ppp: implement rtnetlink device handling
Am 23.12.2015 21:04, schrieb Guillaume Nault:
> Define PPP device handler for use with rtnetlink.
>
> The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
> contains the file descriptor of the associated /dev/ppp instance (the
> file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
> the ioctl-based API).
>
> PPP devices created with the rtnetlink API behave like the ones created
> with ioctl(PPPIOCNEWUNIT). In particular existing ioctls still apply to
> PPP units created with the rtnetlink API.
>
> Signed-off-by: Guillaume Nault
> ---
> drivers/net/ppp/ppp_generic.c | 109
> --
> include/uapi/linux/if_link.h | 8
> 2 files changed, 113 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index bc24537..19476d6 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -46,6 +46,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -185,7 +186,9 @@ struct channel {
>
> struct ppp_config {
> struct file *file;
> + s32 fd;
> s32 unit;
> + bool ifname_is_set;
> };
>
> /*
> @@ -286,6 +289,7 @@ static int unit_get(struct idr *p, void *ptr);
> static int unit_set(struct idr *p, void *ptr, int n);
> static void unit_put(struct idr *p, int n);
> static void *unit_find(struct idr *p, int n);
> +static void ppp_setup(struct net_device *dev);
>
> static const struct net_device_ops ppp_netdev_ops;
>
> @@ -954,7 +958,7 @@ static struct pernet_operations ppp_net_ops = {
> .size = sizeof(struct ppp_net),
> };
>
> -static int ppp_unit_register(struct ppp *ppp, int unit)
> +static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
> {
> struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
> int ret;
> @@ -984,7 +988,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
>
> ppp->file.index = ret;
>
> - snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
> + if (!ifname_is_set)
> + snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
>
> ret = register_netdevice(ppp->dev);
> if (ret)
> @@ -1012,7 +1017,24 @@ static int ppp_dev_configure(struct net *src_net,
> struct net_device *dev,
> int indx;
> int err;
>
> - file = conf->file;
> + if (conf->fd >= 0) {
> + file = fget(conf->fd);
> + if (file) {
> + if (file->f_op != &ppp_device_fops) {
> + fput(file);
> + return -EBADF;
> + }
> +
> + /* Don't hold reference on file: ppp_release() is
> + * responsible for safely freeing the associated
> + * resources upon release. So file won't go away
> + * from under us.
> + */
> + fput(file);
> + }
> + } else {
> + file = conf->file;
> + }
> if (!file)
> return -EBADF;
I would write that a bid different to reduce indent
und improve readability
(note: totaly untested just reviewing)
if (conf->fd < 0) {
file = conf->file;
if (!file)
return -EBADF;
}
else
{
file = fget(conf->fd);
if (!file)
return -EBADF;
fput(file);
if (file->f_op != &ppp_device_fops) {
return -EBADF;
}
}
just my 2 cents
re,
wh
> if (file->private_data)
> @@ -1040,7 +1062,7 @@ static int ppp_dev_configure(struct net *src_net,
> struct net_device *dev,
> ppp->mru = PPP_MRU;
> ppp->ppp_net = src_net;
>
> - err = ppp_unit_register(ppp, conf->unit);
> + err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
> if (err < 0)
> return err;
>
> @@ -1049,6 +1071,73 @@ static int ppp_dev_configure(struct net *src_net,
> struct net_device *dev,
> return 0;
> }
>
> +static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
> + [IFLA_PPP_DEV_FD] = { .type = NLA_S32 },
> +};
> +
> +static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
> +{
> + if (!data)
> + return -EINVAL;
> +
> + if (!data[IFLA_PPP_DEV_FD])
> + return -EINVAL;
> + if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
> + return -EBADF;
> +
> + return 0;
> +}
> +
> +static int ppp_nl_newlink(struct net *src_net, struct net_device *dev,
> + struct nlattr *tb[], struct nlattr *data[])
> +{
> + struct ppp_config conf = {
> + .file = NULL,
> + .unit = -1,
> + .ifname_is_set = true,
> + };
> +
> + conf.fd = nla_get_s32(data[IFLA_PPP_DEV_FD]);
> +
> + return ppp_dev_configure(src_net, dev, &conf);
> +}
> +
> +static void ppp_nl_dellink(struct net_device
Re: [patch] qlcnic: fix a timeout loop
Am 15.12.2015 14:46, schrieb Manish Chopra:
>> -Original Message-
>> From: dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com
>> [mailto:dept_hsg_linux_nic_dev-boun...@qlclistserver.qlogic.com] On Behalf
>> Of Dan Carpenter
>> Sent: Tuesday, December 15, 2015 3:46 PM
>> To: Dept-GE Linux NIC Dev ; Rajesh
>> Borundia
>> Cc: netdev ; kernel-janit...@vger.kernel.org
>> Subject: [patch] qlcnic: fix a timeout loop
>>
>> The problem here is that at the end of the loop we test for if
>> idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a
>> post-op, it actually ends up set to (u8)-1. I have fixed this by changing
>> it to a
>> pre-op. I had to change the starting value from
>> "QLCNIC_DEV_NPAR_OPER_TIMEO" (30) to "QLCNIC_DEV_NPAR_OPER_TIMEO
>> + 1" so that we still loop the same number of times as before.
>>
>> Fixes: 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
>> Signed-off-by: Dan Carpenter
>>
>> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> index be7d7a6..9919245 100644
>> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> @@ -234,7 +234,7 @@ int qlcnic_83xx_config_vnic_opmode(struct
>> qlcnic_adapter *adapter)
>> }
>>
>> ahw->idc.vnic_state = QLCNIC_DEV_NPAR_NON_OPER;
>> -ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;
>> +ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO + 1;
>>
>> return 0;
>> }
>> @@ -246,7 +246,7 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter
>> *adapter)
>> u32 state;
>>
>> state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
>> -while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
>> +while (state != QLCNIC_DEV_NPAR_OPER && --idc->vnic_wait_limit) {
>> msleep(1000);
>> state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
>> }
>
> Hi Dan,
> It looks bit odd incrementing 1 in QLCNIC_DEV_NPAR_OPER_TIMEO. Can't we just
> post increment inside the loop ?
>
> ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;
> while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit) {
> idc->vnic_wait_limit--;
> -;
> -;
> }
>
> Thanks,
> Manish
Hi Manish,
i would like to ask an other question. Why do you choose this way ?
Basicly you have a
#define QLCNIC_DEV_NPAR_OPER_TIMEO
idc->vnic_wait_limit=QLCNIC_DEV_NPAR_OPER_TIMEO;
while ( ... --idc->vnic_wait_limit)
Do you need the time it took to chance the state ?
Look at Dan patches, there is a whole list that shows that programmers are
terrible at counting backwarts. Maybe it is possible to change the code into
something like
while ( cnt++ < idc->vnic_wait_limit)
this way you have a flexible limit, and it is better to understand for others
what you want to archive.
just my 2 cents,
re,
wh
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] decnet: remove macro-local declarations
+1
I like this more since it is much more obvious what is done.
more over we can remove a macro with only 2 users.
re,
wh
Am 06.11.2015 11:57, schrieb Julia Lawall:
>>> Would it be preferable to remove the macro entirely and inline the for
>>> loop header?
>>
>> Could you show me an example of how this would look exactly?
>
> One possible solution is below. I moved the initialization of the nh
> pointer inside the loop to reduce the size of the loop header. One could
> also inline fi->fib_nh[nhsel] where it occurs, but it seemed that that
> would make quite long expressions.
>
> julia
>
> diff --git a/net/decnet/dn_table.c b/net/decnet/dn_table.c
> index 1540b50..509ae82 100644
> --- a/net/decnet/dn_table.c
> +++ b/net/decnet/dn_table.c
> @@ -60,11 +60,6 @@ struct dn_hash
>
> #define dz_key_0(key)((key).datum = 0)
>
> -#define for_nexthops(fi) { int nhsel; const struct dn_fib_nh *nh;\
> - for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)
> -
> -#define endfor_nexthops(fi) }
> -
> #define DN_MAX_DIVISOR 1024
> #define DN_S_ZOMBIE 1
> #define DN_S_ACCESSED 2
> @@ -227,7 +222,7 @@ static struct dn_zone *dn_new_zone(struct dn_hash *table,
> int z)
> static int dn_fib_nh_match(struct rtmsg *r, struct nlmsghdr *nlh, struct
> nlattr *attrs[], struct dn_fib_info *fi)
> {
> struct rtnexthop *nhp;
> - int nhlen;
> + int nhlen, nhsel;
>
> if (attrs[RTA_PRIORITY] &&
> nla_get_u32(attrs[RTA_PRIORITY]) != fi->fib_priority)
> @@ -246,8 +241,9 @@ static int dn_fib_nh_match(struct rtmsg *r, struct
> nlmsghdr *nlh, struct nlattr
> nhp = nla_data(attrs[RTA_MULTIPATH]);
> nhlen = nla_len(attrs[RTA_MULTIPATH]);
>
> - for_nexthops(fi) {
> + for (nhsel = 0; nhsel < fi->fib_nhs; nhsel++) {
> int attrlen = nhlen - sizeof(struct rtnexthop);
> + const struct dn_fib_nh *nh = &fi->fib_nh[nhsel];
> __le16 gw;
>
> if (attrlen < 0 || (nhlen -= nhp->rtnh_len) < 0)
> @@ -264,7 +260,7 @@ static int dn_fib_nh_match(struct rtmsg *r, struct
> nlmsghdr *nlh, struct nlattr
> return 1;
> }
> nhp = RTNH_NEXT(nhp);
> - } endfor_nexthops(fi);
> + }
>
> return 0;
> }
> @@ -345,11 +341,13 @@ static int dn_fib_dump_info(struct sk_buff *skb, u32
> portid, u32 seq, int event,
> if (fi->fib_nhs > 1) {
> struct rtnexthop *nhp;
> struct nlattr *mp_head;
> + int nhsel;
>
> if (!(mp_head = nla_nest_start(skb, RTA_MULTIPATH)))
> goto errout;
>
> - for_nexthops(fi) {
> + for (nhsel = 0; nhsel < fi->fib_nhs; nhsel++) {
> + const struct dn_fib_nh *nh = &fi->fib_nh[nhsel];
> if (!(nhp = nla_reserve_nohdr(skb, sizeof(*nhp
> goto errout;
>
> @@ -362,7 +360,7 @@ static int dn_fib_dump_info(struct sk_buff *skb, u32
> portid, u32 seq, int event,
> goto errout;
>
> nhp->rtnh_len = skb_tail_pointer(skb) - (unsigned char
> *)nhp;
> - } endfor_nexthops(fi);
> + }
>
> nla_nest_end(skb, mp_head);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fib_trie: Fix potential null pointer dereference
Am 06.06.2015 13:35, schrieb Firo Yang:
> A smatch warning.
> When kmem_cache_alloc() failed to alloc memory, a null pointer
> will be returned. Redeference null pointer will generate
> an unnecessary oops. So, use it after check.
>
> Signed-off-by: Firo Yang
> ---
> net/ipv4/fib_trie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 01bce15..34094c7 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
> static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
> {
> struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
> - struct key_vector *l = kv->kv;
> + struct key_vector *l;
It is a good custom to have action and check close together, so this may be more
obvious for future readers:
struct tnode *kv;
struct key_vector *l;
kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
if (!kv)
return NULL;
re,
wh
> /* initialize key vector */
> + l = kv->kv;
> l->key = key;
> l->pos = 0;
> l->bits = 0;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KJ] [tipc-discussion] [patch] net/tipc: sprintf/strcpy conversion
Florian Westphal wrote: > walter harms <[EMAIL PROTECTED]> wrote: >> These line >> + strcpy(bcbearer->media.name, "tipc-multicast"); >> i gues that means tipc_bclink_name ? > > The idea was to change how things are done, not _what_ is being done. > >> an even more secure version could be like this: >> >> >> strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name); > > Ugh, please, no. The size of src is known in all cases; there is > absoluty no point in using str(n|l)cpy here. > >> (in case someone ever changes the size of cbearer->media.name or >> tipc_bclink_name and the hope >> that wchat_t will never reach the kernel) > > In this case 'someone' should be really hurt, don't you think? > hi florian, i am on the side of error, the code increase is marginal and the speed penalty also, so why not ? you make sure that an overflow may never happen, and the rest in name gets zeroed. The problem is that when the error occurs it may be later than the actual changeset. NTL it is an hint, and if you feel ok with it and the maintainer has no objects i have no problems either. re, wh - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
David Miller wrote: > From: walter harms <[EMAIL PROTECTED]> > Date: Wed, 01 Nov 2006 22:38:26 +0100 > >> These line >> +strcpy(bcbearer->media.name, "tipc-multicast"); >> >> i gues that means tipc_bclink_name ? mea culpa, i should not write mail when tired. > > Why? The original code used "tipc-multicast" which is a > different string than tipc_bclink_name which is "multicast-link". > >>> - sprintf(bcbearer->media.name, "tipc-multicast"); >>> + strcpy(bcbearer->media.name, "tipc-multicast"); > > If you are arguing that it should be changed, that's a different > changeset to discuss. > > > - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KJ] [patch] net/tipc: sprintf/strcpy conversion
hi Florian,
These line
+ strcpy(bcbearer->media.name, "tipc-multicast");
i gues that means tipc_bclink_name ?
an even more secure version could be like this:
strncpy(bcbearer->media.name,sizeof(bcbearer->media.name),tipc_bclink_name);
(in case someone ever changes the size of cbearer->media.name or
tipc_bclink_name and the hope
that wchat_t will never reach the kernel)
re,
wh
Florian Westphal wrote:
> From: Florian Westphal <[EMAIL PROTECTED]>
>
> convert sprintf(a,b) to strcpy(a,b). Make tipc_bclink_name[] const.
>
> Signed-off-by: Florian Westphal <[EMAIL PROTECTED]>
>
> ---
>
> compile tested; diffed against davem/net-2.6.
>
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -119,7 +119,7 @@ static struct bclink *bclink = NULL;
> static struct link *bcl = NULL;
> static DEFINE_SPINLOCK(bc_lock);
>
> -char tipc_bclink_name[] = "multicast-link";
> +const char tipc_bclink_name[] = "multicast-link";
>
>
> static u32 buf_seqno(struct sk_buff *buf)
> @@ -790,7 +790,7 @@ int tipc_bclink_init(void)
> INIT_LIST_HEAD(&bcbearer->bearer.cong_links);
> bcbearer->bearer.media = &bcbearer->media;
> bcbearer->media.send_msg = tipc_bcbearer_send;
> - sprintf(bcbearer->media.name, "tipc-multicast");
> + strcpy(bcbearer->media.name, "tipc-multicast");
>
> bcl = &bclink->link;
> memset(bclink, 0, sizeof(struct bclink));
> @@ -802,7 +802,7 @@ int tipc_bclink_init(void)
> tipc_link_set_queue_limits(bcl, BCLINK_WIN_DEFAULT);
> bcl->b_ptr = &bcbearer->bearer;
> bcl->state = WORKING_WORKING;
> - sprintf(bcl->name, tipc_bclink_name);
> + strcpy(bcl->name, tipc_bclink_name);
>
> if (BCLINK_LOG_BUF_SIZE) {
> char *pb = kmalloc(BCLINK_LOG_BUF_SIZE, GFP_ATOMIC);
> diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> --- a/net/tipc/bcast.h
> +++ b/net/tipc/bcast.h
> @@ -70,7 +70,7 @@ struct port_list {
>
> struct node;
>
> -extern char tipc_bclink_name[];
> +extern const char tipc_bclink_name[];
>
>
> /**
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -667,7 +667,7 @@ struct sk_buff *tipc_node_get_links(cons
> link_info.dest = tipc_own_addr & 0xf00;
> link_info.dest = htonl(link_info.dest);
> link_info.up = htonl(1);
> -sprintf(link_info.str, tipc_bclink_name);
> +strcpy(link_info.str, tipc_bclink_name);
> tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info,
> sizeof(link_info));
>
> /* Add TLVs for any other links in scope */
> ___
> Kernel-janitors mailing list
> [EMAIL PROTECTED]
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
