RE: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
Do we need ACCESS_ONCE() here to avoid check/use issues?
-Original Message-
From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
Sent: Wednesday, October 19, 2016 2:53 PM
To: net...@vger.kernel.org
Cc: Stephen Hemminger ; de...@linuxdriverproject.org;
linux-ker...@vger.kernel.org; KY Srinivasan ; Haiyang Zhang
Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and
netvsc_init_buf()
Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
chn_table") turns out to be incomplete. A crash in
netvsc_get_next_send_section() is observed on mtu change when the device is
under load. The race I identified is: if we get to netvsc_send() after we set
net_device_ctx->nvdev link in netvsc_device_add() but before we finish
netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and
we crash. Unfortunately we can't set net_device_ctx->nvdev link after the
netvsc_init_buf() call as during the negotiation we need to receive packets and
on the receive path we check for it. It would probably be possible to split
nvdev into a pair of nvdev_in and nvdev_out links and check them accordingly in
get_outbound_net_device()/
get_inbound_net_device() but this looks like an overkill.
Check that send_section_map is allocated in netvsc_send().
Signed-off-by: Vitaly Kuznetsov
---
drivers/net/hyperv/netvsc.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index
720b5fa..e2bfaac 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
if (!net_device)
return -ENODEV;
+ /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
+* here before the negotiation with the host is finished and
+* send_section_map may not be allocated yet.
+*/
+ if (!net_device->send_section_map)
+ return -EAGAIN;
+
out_channel = net_device->chn_table[q_idx];
packet->send_buf_index = NETVSC_INVALID_INDEX;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/37] staging:r8188eu: remove skb cloning after netdev_alloc_skb fail
On Wed, Oct 19, 2016 at 10:07:31PM +0700, Ivan Safonov wrote: > In accordance with the KISS principle. > The changelog is not complete enough. Some people are going to read only the subject, but other people are only going to read the body of the changelog. They can be far apart depending on how you look at patches. For example: http://marc.info/?l=linux-driver-devel&m=147688960328996&w=2 It should say: "Just drop the packet if we can't allocate data. Don't go to a lot of work to try recover. Let the other side resend the data." regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/37] staging:r8188eu: remove device assignment after netdev_alloc_skb call
On Wed, Oct 19, 2016 at 10:07:30PM +0700, Ivan Safonov wrote: > netdev_alloc_skb function > already set dev member of pkt_copy. > I'm coming back to this one after sending a response to patch 3. Here again, this patch was complicated for me to review because I mostly read the body of the email and not the subject. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
Stephen Hemminger writes:
> Do we need ACCESS_ONCE() here to avoid check/use issues?
>
I think we don't: this is the only place in the function where we read
the variable so we'll get normal read. We're not trying to syncronize
with netvsc_init_buf() as that would require locking, if we read stale
NULL value after it was already updated on a different CPU we're fine,
we'll just return -EAGAIN.
> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Wednesday, October 19, 2016 2:53 PM
> To: net...@vger.kernel.org
> Cc: Stephen Hemminger ; de...@linuxdriverproject.org;
> linux-ker...@vger.kernel.org; KY Srinivasan ; Haiyang
> Zhang
> Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and
> netvsc_init_buf()
>
> Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
> chn_table") turns out to be incomplete. A crash in
> netvsc_get_next_send_section() is observed on mtu change when the device is
> under load. The race I identified is: if we get to netvsc_send() after we set
> net_device_ctx->nvdev link in netvsc_device_add() but before we finish
> netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and
> we crash. Unfortunately we can't set net_device_ctx->nvdev link after the
> netvsc_init_buf() call as during the negotiation we need to receive packets
> and on the receive path we check for it. It would probably be possible to
> split nvdev into a pair of nvdev_in and nvdev_out links and check them
> accordingly in get_outbound_net_device()/
> get_inbound_net_device() but this looks like an overkill.
>
> Check that send_section_map is allocated in netvsc_send().
>
> Signed-off-by: Vitaly Kuznetsov
> ---
> drivers/net/hyperv/netvsc.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index
> 720b5fa..e2bfaac 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
> if (!net_device)
> return -ENODEV;
>
> + /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
> + * here before the negotiation with the host is finished and
> + * send_section_map may not be allocated yet.
> + */
> + if (!net_device->send_section_map)
> + return -EAGAIN;
> +
> out_channel = net_device->chn_table[q_idx];
>
> packet->send_buf_index = NETVSC_INVALID_INDEX;
> --
> 2.7.4
--
Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/37] staging:r8188eu: update pkt->data synchronously with rx_data
On Wed, Oct 19, 2016 at 10:07:45PM +0700, Ivan Safonov wrote: > To replace rx_data with pkt->data. > Now pkt->data is equal to rx_data. > I'm not smart enough to tell on my own... Is this a bugfix? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 29/37] staging:r8188eu: take out stripping of iv and icv space from wlanhdr_to_ethhdr function
On Wed, Oct 19, 2016 at 10:07:57PM +0700, Ivan Safonov wrote:
> @@ -1733,6 +1729,13 @@ static int recv_indicatepkts_in_order(struct adapter
> *padapter, struct recv_reor
> return bPktInBuf;
> }
>
> +void strip_iv_icv(struct sk_buff *skb, uint header_len,
> + uint iv_len, uint icv_len) {
Curly braces go down a line.
> + memmove(skb->data + iv_len, skb->data, header_len);
> + skb_pull(skb, iv_len);
> + skb_trim(skb, skb->len - icv_len);
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/37] staging:r8188eu: remove get_rxmem function
I don't know exactly what the game plan is for the rtl8188eu driver but these patches seem to be on the right path to me. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[bug report] staging: lustre: clio: Revise read ahead implementation
Hello Jinshan Xiong,
The patch 1e1db2a97be5: "staging: lustre: clio: Revise read ahead
implementation" from Oct 2, 2016, leads to the following static
checker warning:
drivers/staging/lustre/lustre/lov/lov_io.c:611 lov_io_read_ahead()
error: 'sub' dereferencing possible ERR_PTR()
drivers/staging/lustre/lustre/lov/lov_io.c
589 static int lov_io_read_ahead(const struct lu_env *env,
590 const struct cl_io_slice *ios,
591 pgoff_t start, struct cl_read_ahead *ra)
592 {
593 struct lov_io *lio = cl2lov_io(env, ios);
594 struct lov_object *loo = lio->lis_object;
595 struct cl_object *obj = lov2cl(loo);
596 struct lov_layout_raid0 *r0 = lov_r0(loo);
597 unsigned int pps; /* pages per stripe */
598 struct lov_io_sub *sub;
599 pgoff_t ra_end;
600 loff_t suboff;
601 int stripe;
602 int rc;
603
604 stripe = lov_stripe_number(loo->lo_lsm, cl_offset(obj, start));
605 if (unlikely(!r0->lo_sub[stripe]))
606 return -EIO;
607
608 sub = lov_sub_get(env, lio, stripe);
^^^
Do we need error handling here?
609
610 lov_stripe_offset(loo->lo_lsm, cl_offset(obj, start), stripe,
&suboff);
611 rc = cl_io_read_ahead(sub->sub_env, sub->sub_io,
612cl_index(lovsub2cl(r0->lo_sub[stripe]),
suboff),
613ra);
614 lov_sub_put(sub);
615
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/37] staging:r8188eu: remove device assignment after netdev_alloc_skb call
Andy,
thank you for reviewing patches and tips.
On 10/19/2016 11:59 PM, Andy Shevchenko wrote:
On Wed, 2016-10-19 at 19:53 +0300, Andy Shevchenko wrote:
On Wed, 2016-10-19 at 22:07 +0700, Ivan Safonov wrote:
netdev_alloc_skb function
already set dev member of pkt_copy.
It might be one line.
Some special references:
functions: function()
structs: struct my_struct
P.S. Entire series lacks of cover letter.
And couple more things:
- Use min percentage when run git format-patch to avoid inclusion non-
maintainers / non-active developers
- Adjust your commit messages to be a bit more verbose and put
explanation "why you create the patch" before "what is done in the
patch".
Signed-off-by: Ivan Safonov
---
drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
index 34198fe..f19b203 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
@@ -123,7 +123,6 @@ static int recvbuf2recvframe(struct adapter
*adapt, struct sk_buff *pskb)
pkt_copy = netdev_alloc_skb(adapt->pnetdev,
alloc_sz);
if (pkt_copy) {
- pkt_copy->dev = adapt->pnetdev;
precvframe->pkt = pkt_copy;
precvframe->rx_head = pkt_copy->data;
precvframe->rx_end = pkt_copy->data +
alloc_sz;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[bug report] staging: lustre: llite: remove duplicate fiemap defines
Hello Bobi Jam,
The patch cbd4d4a8e319: "staging: lustre: llite: remove duplicate
fiemap defines" from Oct 2, 2016, leads to the following static
checker warning:
drivers/staging/lustre/lustre/lov/lov_object.c:1241 lov_object_fiemap()
warn: signed overflow undefined. 'fm_start + fm_length < fm_start'
drivers/staging/lustre/lustre/lov/lov_object.c
1102 static int lov_object_fiemap(const struct lu_env *env, struct cl_object
*obj,
1103 struct ll_fiemap_info_key *fmkey,
1104 struct fiemap *fiemap, size_t *buflen)
1105 {
1106 struct lov_obd *lov = lu2lov_dev(obj->co_lu.lo_dev)->ld_lov;
1107 unsigned int buffer_size = FIEMAP_BUFFER_SIZE;
1108 struct fiemap_extent *lcl_fm_ext;
1109 struct cl_object *subobj = NULL;
1110 struct fiemap *fm_local = NULL;
struct lov_stripe_md *lsm;
1112 loff_t fm_start;
1113 loff_t fm_end;
1114 loff_t fm_length;
^^
These used to be u64 types but now they are signed.
1115 loff_t fm_end_offset;
1116 int count_local;
1117 int ost_index = 0;
1118 int start_stripe;
1119 int current_extent = 0;
1120 int rc = 0;
1121 int last_stripe;
1122 int cur_stripe = 0;
1123 int cur_stripe_wrap = 0;
1124 int stripe_count;
1125 /* Whether have we collected enough extents */
1126 bool enough = false;
1127 /* EOF for object */
1128 bool ost_eof = false;
1129 /* done with required mapping for this OST? */
1130 bool ost_done = false;
1131
[ snip ]
1231 /*
1232 * If this is a continuation FIEMAP call and we are on
1233 * starting stripe then lun_start needs to be set to
1234 * fm_end_offset
1235 */
1236 if (fm_end_offset && cur_stripe == start_stripe)
1237 lun_start = fm_end_offset;
1238
1239 if (fm_length != ~0ULL) {
1240 /* Handle fm_start + fm_length overflow */
1241 if (fm_start + fm_length < fm_start)
^^^
So, technically, this is undefined behavior now.
1242 fm_length = ~0ULL - fm_start;
1243 lun_end = lov_size_to_stripe(lsm, fm_start +
fm_length,
1244 cur_stripe);
1245 } else {
1246 lun_end = ~0ULL;
1247 }
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: xgifb: Fix NULL pointer comparison warning
Replace direct comparisons to NULL i.e.
'x == NULL' with '!x'.
This problem was detected by checkpatch.
Signed-off-by: Maninder Singh
---
drivers/staging/xgifb/XGI_main_26.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c
b/drivers/staging/xgifb/XGI_main_26.c
index 0c78491..89bd4dd 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -518,7 +518,7 @@ static void XGIfb_search_crt2type(const char *name)
{
int i = 0;
- if (name == NULL)
+ if (!name)
return;
while (XGI_crt2type[i].type_no != -1) {
@@ -589,7 +589,7 @@ static void XGIfb_search_tvstd(const char *name)
{
int i = 0;
- if (name == NULL)
+ if (!name)
return;
while (XGI_tvtype[i].type_no != -1) {
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 17/37] staging:r8188eu: update pkt->data synchronously with rx_data
On 10/20/2016 04:03 PM, Dan Carpenter wrote: On Wed, Oct 19, 2016 at 10:07:45PM +0700, Ivan Safonov wrote: To replace rx_data with pkt->data. Now pkt->data is equal to rx_data. I'm not smart enough to tell on my own... Is this a bugfix? It is not bugfix, only variable replacement. recv_frame structure duplicates sk_buff. Most of the ieee80211_* functions require a pointer to sk_buff. Therefore, easier to use sk_buff instead of recv_frame structure. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/37] staging:r8188eu: remove get_rxmem function
On 10/20/2016 04:18 PM, Dan Carpenter wrote: I don't know exactly what the game plan is for the rtl8188eu driver but these patches seem to be on the right path to me. regards, dan carpenter If this driver has no future and will be removed, then what code is better to invest my efforts? Linux programming is only my hobby and point in CV. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: lustre: clio: Revise read ahead implementation
> On Oct 20, 2016, at 7:17 PM, Dan Carpenter wrote:
>
> Hello Jinshan Xiong,
>
> The patch 1e1db2a97be5: "staging: lustre: clio: Revise read ahead
> implementation" from Oct 2, 2016, leads to the following static
> checker warning:
>
> drivers/staging/lustre/lustre/lov/lov_io.c:611 lov_io_read_ahead()
> error: 'sub' dereferencing possible ERR_PTR()
>
> drivers/staging/lustre/lustre/lov/lov_io.c
> 589 static int lov_io_read_ahead(const struct lu_env *env,
> 590 const struct cl_io_slice *ios,
> 591 pgoff_t start, struct cl_read_ahead *ra)
> 592 {
> 593 struct lov_io *lio = cl2lov_io(env, ios);
> 594 struct lov_object *loo = lio->lis_object;
> 595 struct cl_object *obj = lov2cl(loo);
> 596 struct lov_layout_raid0 *r0 = lov_r0(loo);
> 597 unsigned int pps; /* pages per stripe */
> 598 struct lov_io_sub *sub;
> 599 pgoff_t ra_end;
> 600 loff_t suboff;
> 601 int stripe;
> 602 int rc;
> 603
> 604 stripe = lov_stripe_number(loo->lo_lsm, cl_offset(obj, start));
> 605 if (unlikely(!r0->lo_sub[stripe]))
> 606 return -EIO;
> 607
> 608 sub = lov_sub_get(env, lio, stripe);
>^^^
> Do we need error handling here?
Indeed, this needs error handling. Thanks for pointing it out. I will push a
patch soon.
Jinshan
>
> 609
> 610 lov_stripe_offset(loo->lo_lsm, cl_offset(obj, start), stripe,
> &suboff);
> 611 rc = cl_io_read_ahead(sub->sub_env, sub->sub_io,
> 612cl_index(lovsub2cl(r0->lo_sub[stripe]),
> suboff),
> 613ra);
> 614 lov_sub_put(sub);
> 615
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: Corrected a spelling mistake
This patch corrects the spelling of 'initialize' in ks7010_sdio.c. The issue was found by checkpatch. Signed-off-by: Nick Rosbrook --- drivers/staging/ks7010/ks7010_sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c index b02980d..039efce 100644 --- a/drivers/staging/ks7010/ks7010_sdio.c +++ b/drivers/staging/ks7010/ks7010_sdio.c @@ -956,7 +956,7 @@ static int ks7010_sdio_probe(struct sdio_func *func, priv = NULL; netdev = NULL; - /* initilize ks_sdio_card */ + /* initialize ks_sdio_card */ card = kzalloc(sizeof(*card), GFP_KERNEL); if (!card) return -ENOMEM; -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
From: Vitaly Kuznetsov Date: Thu, 20 Oct 2016 10:51:04 +0200 > Stephen Hemminger writes: > >> Do we need ACCESS_ONCE() here to avoid check/use issues? >> > > I think we don't: this is the only place in the function where we read > the variable so we'll get normal read. We're not trying to syncronize > with netvsc_init_buf() as that would require locking, if we read stale > NULL value after it was already updated on a different CPU we're fine, > we'll just return -EAGAIN. The concern is if we race with netvsc_destroy_buf() and this pointer becomes NULL after the test you are adding. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: lustre: clio: Revise read ahead implementation
This is Smatch stuff. Some of it requires that the database be built and some is stuff I'm still working on and haven't released yet. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
