* Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
From: Johan Hovold @ 2018-04-11 7:07 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: devel, samuel, gregkh, johan, linux-kernel, netdev,
arvind.yadav.cs, davem
In-Reply-To: <1523410174-1553-1-git-send-email-baijiaju1990@gmail.com>
On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote:
> stir421x_fw_upload() is never called in atomic context.
>
> The call chain ending up at stir421x_fw_upload() is:
> [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe()
>
> irda_usb_probe() is set as ".probe" in struct usb_driver.
> This function is not called in atomic context.
>
> Despite never getting called from atomic context, stir421x_fw_upload()
> calls mdelay() to busily wait.
> This is not necessary and can be replaced with usleep_range() to
> avoid busy waiting.
>
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
This all looks good (also note the call to usb_control_msg(), which may
sleep, just above the mdelay()).
Reviewed-by: Johan Hovold <johan@kernel.org>
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
From: Greg KH @ 2018-04-11 6:41 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: devel, samuel, netdev, johan, linux-kernel, arvind.yadav.cs,
davem
In-Reply-To: <1523410174-1553-1-git-send-email-baijiaju1990@gmail.com>
On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote:
> stir421x_fw_upload() is never called in atomic context.
>
> The call chain ending up at stir421x_fw_upload() is:
> [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe()
>
> irda_usb_probe() is set as ".probe" in struct usb_driver.
> This function is not called in atomic context.
>
> Despite never getting called from atomic context, stir421x_fw_upload()
> calls mdelay() to busily wait.
> This is not necessary and can be replaced with usleep_range() to
> avoid busy waiting.
>
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
> drivers/staging/irda/drivers/irda-usb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Please, at the very least, work off of Linus's tree. There is no
drivers/staging/irda/ anymore :)
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2018-04-11 6:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, jasowang,
loseweigh
In-Reply-To: <20180411060327.GH2028@nanopsycho>
On 4/10/2018 11:03 PM, Jiri Pirko wrote:
> Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
>> On 4/10/2018 8:43 AM, Jiri Pirko wrote:
>>> Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
>>>> On 4/10/2018 8:22 AM, Jiri Pirko wrote:
>>>>> Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>> On 4/10/2018 3:55 AM, Jiri Pirko wrote:
>>>>>>> Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>> On 4/9/2018 1:07 AM, Jiri Pirko wrote:
>>>>>>>>> Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>>>> On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>>>>>>>>>>> Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>>> +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>>>>>>>>>>>> + struct net_device *child_netdev)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + struct virtnet_bypass_info *vbi;
>>>>>>>>>>>> + bool backup;
>>>>>>>>>>>> +
>>>>>>>>>>>> + vbi = netdev_priv(bypass_netdev);
>>>>>>>>>>>> + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>>>>>>>>>>>> + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>>>>>>>>>>>> + rtnl_dereference(vbi->active_netdev)) {
>>>>>>>>>>>> + netdev_info(bypass_netdev,
>>>>>>>>>>>> + "%s attempting to join bypass dev when %s already present\n",
>>>>>>>>>>>> + child_netdev->name, backup ? "backup" : "active");
>>>>>>>>>>> Bypass module should check if there is already some other netdev
>>>>>>>>>>> enslaved and refuse right there.
>>>>>>>>>> This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>>>>>>>>>> as its bypass_netdev is same as the backup_netdev.
>>>>>>>>>> Will add a flag while registering with the bypass module to indicate if the driver is doing
>>>>>>>>>> a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>>>>>>>>>> for 3 netdev scenario.
>>>>>>>>> Just let me undestand it clearly. What I expect the difference would be
>>>>>>>>> between 2netdev and3 netdev model is this:
>>>>>>>>> 2netdev:
>>>>>>>>> bypass_master
>>>>>>>>> /
>>>>>>>>> /
>>>>>>>>> VF_slave
>>>>>>>>>
>>>>>>>>> 3netdev:
>>>>>>>>> bypass_master
>>>>>>>>> / \
>>>>>>>>> / \
>>>>>>>>> VF_slave backup_slave
>>>>>>>>>
>>>>>>>>> Is that correct? If not, how does it look like?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Looks correct.
>>>>>>>> VF_slave and backup_slave are the original netdevs and are present in both the models.
>>>>>>>> In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
>>>>>>>> marked as the 2 slaves of this new netdev.
>>>>>>> You say it looks correct and in another sentence you provide completely
>>>>>>> different description. Could you please look again?
>>>>>>>
>>>>>> To be exact, 2 netdev model with netvsc looks like this.
>>>>>>
>>>>>> netvsc_netdev
>>>>>> /
>>>>>> /
>>>>>> VF_slave
>>>>>>
>>>>>> With virtio_net, 3 netdev model
>>>>>>
>>>>>> bypass_netdev
>>>>>> / \
>>>>>> / \
>>>>>> VF_slave virtio_net netdev
>>>>> Could you also mark the original netdev which is there now? is it
>>>>> bypass_netdev or virtio_net_netdev ?
>>>> bypass_netdev
>>>> / \
>>>> / \
>>>> VF_slave virtio_net netdev (original)
>>> That does not make sense.
>>> 1) You diverge from the behaviour of the netvsc, where the original
>>> netdev is a master of the VF
>>> 2) If the original netdev is a slave, you cannot have any IP address
>>> configured on it (well you could, but the rx_handler would eat every
>>> incoming packet). So you will break the user bacause he would have to
>>> move the configuration to the new master device.
>>> This only makes sense that the original netdev becomes the master for both
>>> netvsc and virtio_net.
>> Forgot to mention that bypass_netdev takes over the name of the original
>> netdev and
>> virtio_net netdev will get the backup name.
> What do you mean by "name"?
bypass_netdev also is associated with the same pci device as the original virtio_net
netdev via SET_NETDEV_DEV(). Also, we added ndo_get_phys_port_name() to virtio_net
that will return _bkup when BACKUP feature is enabled.
So for ex: if virtio_net inteface was getting 'ens12' as the name assigned by udev
without BACKUP feature, when BACKUP feature is enabled, the bypass_netdev will be
named 'ens12' and the original virtio_net will get named as ens12n_bkup.
>
>> So the userspace network configuration doesn't need to change.
>>
>>
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in pfkey_add
From: Kevin Easton @ 2018-04-11 6:18 UTC (permalink / raw)
To: Eric Biggers
Cc: davem, herbert, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs, syzbot
In-Reply-To: <20180409055636.GA2551@la.guarana.org>
On Mon, Apr 09, 2018 at 01:56:36AM -0400, Kevin Easton wrote:
> On Sun, Apr 08, 2018 at 09:04:33PM -0700, Eric Biggers wrote:
> ...
> >
> > Looks like this is going to be fixed by
> > https://patchwork.kernel.org/patch/10327883/ ("af_key: Always verify length of
> > provided sadb_key"), but it's not applied yet to the ipsec tree yet. Kevin, for
> > future reference, for syzbot bugs it would be helpful to reply to the original
> > bug report and say that a patch was sent out, or even better send the patch as a
> > reply to the bug report email, e.g.
> >
> > git format-patch --in-reply-to="<001a114292fadd3e2505607060a8@google.com>"
> >
> > for this one (and the Message ID can be found in the syzkaller-bugs archive even
> > if the email isn't in your inbox).
>
> Sure, I can do that.
I recalled one reason I _didn't_ do this - the message ID is retrievable
from the archived email, but because the archive is Google Groups the
message recipients aren't (only masked).
- Kevin
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Jiri Pirko @ 2018-04-11 6:03 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, jasowang,
loseweigh
In-Reply-To: <82f741a2-2512-39de-84c6-874f126c27ea@intel.com>
Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
>On 4/10/2018 8:43 AM, Jiri Pirko wrote:
>> Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
>> > On 4/10/2018 8:22 AM, Jiri Pirko wrote:
>> > > Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > On 4/10/2018 3:55 AM, Jiri Pirko wrote:
>> > > > > Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > On 4/9/2018 1:07 AM, Jiri Pirko wrote:
>> > > > > > > Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > > > On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>> > > > > > > > > Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > > [...]
>> > > > > > >
>> > > > > > > > > > +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>> > > > > > > > > > + struct net_device *child_netdev)
>> > > > > > > > > > +{
>> > > > > > > > > > + struct virtnet_bypass_info *vbi;
>> > > > > > > > > > + bool backup;
>> > > > > > > > > > +
>> > > > > > > > > > + vbi = netdev_priv(bypass_netdev);
>> > > > > > > > > > + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>> > > > > > > > > > + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>> > > > > > > > > > + rtnl_dereference(vbi->active_netdev)) {
>> > > > > > > > > > + netdev_info(bypass_netdev,
>> > > > > > > > > > + "%s attempting to join bypass dev when %s already present\n",
>> > > > > > > > > > + child_netdev->name, backup ? "backup" : "active");
>> > > > > > > > > Bypass module should check if there is already some other netdev
>> > > > > > > > > enslaved and refuse right there.
>> > > > > > > > This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>> > > > > > > > as its bypass_netdev is same as the backup_netdev.
>> > > > > > > > Will add a flag while registering with the bypass module to indicate if the driver is doing
>> > > > > > > > a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>> > > > > > > > for 3 netdev scenario.
>> > > > > > > Just let me undestand it clearly. What I expect the difference would be
>> > > > > > > between 2netdev and3 netdev model is this:
>> > > > > > > 2netdev:
>> > > > > > > bypass_master
>> > > > > > > /
>> > > > > > > /
>> > > > > > > VF_slave
>> > > > > > >
>> > > > > > > 3netdev:
>> > > > > > > bypass_master
>> > > > > > > / \
>> > > > > > > / \
>> > > > > > > VF_slave backup_slave
>> > > > > > >
>> > > > > > > Is that correct? If not, how does it look like?
>> > > > > > >
>> > > > > > >
>> > > > > > Looks correct.
>> > > > > > VF_slave and backup_slave are the original netdevs and are present in both the models.
>> > > > > > In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
>> > > > > > marked as the 2 slaves of this new netdev.
>> > > > > You say it looks correct and in another sentence you provide completely
>> > > > > different description. Could you please look again?
>> > > > >
>> > > > To be exact, 2 netdev model with netvsc looks like this.
>> > > >
>> > > > netvsc_netdev
>> > > > /
>> > > > /
>> > > > VF_slave
>> > > >
>> > > > With virtio_net, 3 netdev model
>> > > >
>> > > > bypass_netdev
>> > > > / \
>> > > > / \
>> > > > VF_slave virtio_net netdev
>> > > Could you also mark the original netdev which is there now? is it
>> > > bypass_netdev or virtio_net_netdev ?
>> > bypass_netdev
>> > / \
>> > / \
>> > VF_slave virtio_net netdev (original)
>> That does not make sense.
>> 1) You diverge from the behaviour of the netvsc, where the original
>> netdev is a master of the VF
>> 2) If the original netdev is a slave, you cannot have any IP address
>> configured on it (well you could, but the rx_handler would eat every
>> incoming packet). So you will break the user bacause he would have to
>> move the configuration to the new master device.
>> This only makes sense that the original netdev becomes the master for both
>> netvsc and virtio_net.
>Forgot to mention that bypass_netdev takes over the name of the original
>netdev and
>virtio_net netdev will get the backup name.
What do you mean by "name"?
>So the userspace network configuration doesn't need to change.
>
>
^ permalink raw reply
* Re: KMSAN: uninit-value in tipc_subscrb_rcv_cb
From: syzbot @ 2018-04-11 5:47 UTC (permalink / raw)
To: davem, jon.maloy, linux-kernel, netdev, syzkaller-bugs,
tipc-discussion, ying.xue
In-Reply-To: <001a113e8e9c15014705695813be@google.com>
syzbot has found reproducer for the following crash on
https://github.com/google/kmsan.git/master commit
35ff515e4bda2646f6c881d33951c306ea9c282a (Tue Apr 10 08:59:43 2018 +0000)
Merge pull request #11 from parkerduckworth/readme
syzbot dashboard link:
https://syzkaller.appspot.com/bug?extid=75e6e042c5bbf691fc82
So far this crash happened 3 times on
https://github.com/google/kmsan.git/master.
C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6676653019234304
syzkaller reproducer:
https://syzkaller.appspot.com/x/repro.syz?id=5693411524870144
Raw console output:
https://syzkaller.appspot.com/x/log.txt?id=5043527943716864
Kernel config:
https://syzkaller.appspot.com/x/.config?id=6627248707860932248
compiler: clang version 7.0.0 (trunk 329391)
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+75e6e042c5bbf691fc82@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed.
==================================================================
BUG: KMSAN: uninit-value in htohl net/tipc/subscr.c:66 [inline]
BUG: KMSAN: uninit-value in tipc_subscrb_rcv_cb+0x418/0xe80
net/tipc/subscr.c:339
CPU: 0 PID: 19 Comm: kworker/u4:1 Not tainted 4.16.0+ #83
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: tipc_rcv tipc_recv_work
Call Trace:
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x185/0x1d0 lib/dump_stack.c:53
kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
__msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:676
htohl net/tipc/subscr.c:66 [inline]
tipc_subscrb_rcv_cb+0x418/0xe80 net/tipc/subscr.c:339
tipc_receive_from_sock+0x64c/0x800 net/tipc/server.c:271
tipc_recv_work+0xd8/0x1f0 net/tipc/server.c:618
process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2113
worker_thread+0x113c/0x24f0 kernel/workqueue.c:2247
kthread+0x539/0x720 kernel/kthread.c:239
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:406
Uninit was created at:
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:278 [inline]
kmsan_internal_poison_shadow+0xb8/0x1b0 mm/kmsan/kmsan.c:188
kmsan_kmalloc+0x94/0x100 mm/kmsan/kmsan.c:314
kmem_cache_alloc+0xaab/0xb90 mm/slub.c:2756
tipc_receive_from_sock+0x15c/0x800 net/tipc/server.c:253
tipc_recv_work+0xd8/0x1f0 net/tipc/server.c:618
process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2113
worker_thread+0x113c/0x24f0 kernel/workqueue.c:2247
kthread+0x539/0x720 kernel/kthread.c:239
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:406
==================================================================
^ permalink raw reply
* Re: [PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio
From: Phil Reid @ 2018-04-11 5:30 UTC (permalink / raw)
To: Jia-Ju Bai, f.fainelli, andrew, vivien.didelot; +Cc: netdev, linux-kernel
In-Reply-To: <1523411485-2030-1-git-send-email-baijiaju1990@gmail.com>
On 11/04/2018 09:51, Jia-Ju Bai wrote:
> b53_switch_reset_gpio() is never called in atomic context.
>
> The call chain ending up at b53_switch_reset_gpio() is:
> [1] b53_switch_reset_gpio() <- b53_switch_reset() <-
> b53_reset_switch() <- b53_setup()
>
> b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops.
> This function is not called in atomic context.
>
> Despite never getting called from atomic context, b53_switch_reset_gpio()
> calls mdelay() to busily wait.
> This is not necessary and can be replaced with msleep() to
> avoid busy waiting.
>
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
> drivers/net/dsa/b53/b53_common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 274f367..e070ff6 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
> /* Reset sequence: RESET low(50ms)->high(20ms)
> */
> gpio_set_value(gpio, 0);
> - mdelay(50);
> + msleep(50);
>
> gpio_set_value(gpio, 1);
> - mdelay(20);
> + msleep(20);
>
> dev->current_page = 0xff;
> }
>
Would that also imply gpio_set_value could be gpio_set_value_cansleep?
--
Regards
Phil Reid
^ permalink raw reply
* [PATCH net 6/6] bnxt_en: Fix NULL pointer dereference at bnxt_free_irq().
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
When open fails during ethtool -L ring change, for example, the driver
may crash at bnxt_free_irq() because bp->bnapi is NULL.
If we fail to allocate all the new rings, bnxt_open_nic() will free
all the memory including bp->bnapi. Subsequent call to bnxt_close_nic()
will try to dereference bp->bnapi in bnxt_free_irq().
Fix it by checking for !bp->bnapi in bnxt_free_irq().
Fixes: e5811b8c09df ("bnxt_en: Add IRQ remapping logic.")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 9cb8b4b..f83769d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6090,7 +6090,7 @@ static void bnxt_free_irq(struct bnxt *bp)
free_irq_cpu_rmap(bp->dev->rx_cpu_rmap);
bp->dev->rx_cpu_rmap = NULL;
#endif
- if (!bp->irq_tbl)
+ if (!bp->irq_tbl || !bp->bnapi)
return;
for (i = 0; i < bp->cp_nr_rings; i++) {
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 4/6] bnxt_en: Support max-mtu with VF-reps
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev, Sriharsha Basavapatna
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
While a VF is configured with a bigger mtu (> 1500), any packets that
are punted to the VF-rep (slow-path) get dropped by OVS kernel-datapath
with the following message: "dropped over-mtu packet". Fix this by
returning the max-mtu value for a VF-rep derived from its corresponding VF.
VF-rep's mtu can be changed using 'ip' command as shown in this example:
Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 30 +++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 2629040..38f635c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -64,6 +64,31 @@ static int hwrm_cfa_vfr_free(struct bnxt *bp, u16 vf_idx)
return rc;
}
+static int bnxt_hwrm_vfr_qcfg(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
+ u16 *max_mtu)
+{
+ struct hwrm_func_qcfg_output *resp = bp->hwrm_cmd_resp_addr;
+ struct hwrm_func_qcfg_input req = {0};
+ u16 mtu;
+ int rc;
+
+ bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_QCFG, -1, -1);
+ req.fid = cpu_to_le16(bp->pf.vf[vf_rep->vf_idx].fw_fid);
+
+ mutex_lock(&bp->hwrm_cmd_lock);
+
+ rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+ if (!rc) {
+ mtu = le16_to_cpu(resp->max_mtu_configured);
+ if (!mtu)
+ *max_mtu = BNXT_MAX_MTU;
+ else
+ *max_mtu = mtu;
+ }
+ mutex_unlock(&bp->hwrm_cmd_lock);
+ return rc;
+}
+
static int bnxt_vf_rep_open(struct net_device *dev)
{
struct bnxt_vf_rep *vf_rep = netdev_priv(dev);
@@ -365,6 +390,7 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
struct net_device *dev)
{
struct net_device *pf_dev = bp->dev;
+ u16 max_mtu;
dev->netdev_ops = &bnxt_vf_rep_netdev_ops;
dev->ethtool_ops = &bnxt_vf_rep_ethtool_ops;
@@ -380,6 +406,10 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
bnxt_vf_rep_eth_addr_gen(bp->pf.mac_addr, vf_rep->vf_idx,
dev->perm_addr);
ether_addr_copy(dev->dev_addr, dev->perm_addr);
+ /* Set VF-Rep's max-mtu to the corresponding VF's max-mtu */
+ if (!bnxt_hwrm_vfr_qcfg(bp, vf_rep, &max_mtu))
+ dev->max_mtu = max_mtu;
+ dev->min_mtu = ETH_ZLEN;
}
static int bnxt_pcie_dsn_get(struct bnxt *bp, u8 dsn[])
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 5/6] bnxt_en: Need to include RDMA rings in bnxt_check_rings().
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
With recent changes to reserve both L2 and RDMA rings, we need to include
the RDMA rings in bnxt_check_rings(). Otherwise we will under-estimate
the rings we need during ethtool -L and may lead to failure.
Fixes: fbcfc8e46741 ("bnxt_en: Reserve completion rings and MSIX for bnxt_re RDMA driver.")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 1991f0c..9cb8b4b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7686,6 +7686,8 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
if (bp->flags & BNXT_FLAG_AGG_RINGS)
rx_rings <<= 1;
cp = sh ? max_t(int, tx_rings_needed, rx) : tx_rings_needed + rx;
+ if (bp->flags & BNXT_FLAG_NEW_RM)
+ cp += bnxt_get_ulp_msix_num(bp);
return bnxt_hwrm_check_rings(bp, tx_rings_needed, rx_rings, rx, cp,
vnics);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 3/6] bnxt_en: Ignore src port field in decap filter nodes
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev, Sriharsha Basavapatna
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
The driver currently uses src port field (along with other fields) in the
decap tunnel key, while looking up and adding tunnel nodes. This leads to
redundant cfa_decap_filter_alloc() requests to the FW and flow-miss in the
flow engine. Fix this by ignoring the src port field in decap tunnel nodes.
Fixes: f484f6782e01 ("bnxt_en: add hwrm FW cmds for cfa_encap_record and decap_filter")
Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index ac193408..795f450 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1051,8 +1051,10 @@ static int bnxt_tc_get_decap_handle(struct bnxt *bp, struct bnxt_tc_flow *flow,
/* Check if there's another flow using the same tunnel decap.
* If not, add this tunnel to the table and resolve the other
- * tunnel header fileds
+ * tunnel header fileds. Ignore src_port in the tunnel_key,
+ * since it is not required for decap filters.
*/
+ decap_key->tp_src = 0;
decap_node = bnxt_tc_get_tunnel_node(bp, &tc_info->decap_table,
&tc_info->decap_ht_params,
decap_key);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 2/6] bnxt_en: do not allow wildcard matches for L2 flows
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev, Andy Gospodarek
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
From: Andy Gospodarek <gospo@broadcom.com>
Before this patch the following commands would succeed as far as the
user was concerned:
$ tc qdisc add dev p1p1 ingress
$ tc filter add dev p1p1 parent ffff: protocol all \
flower skip_sw action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
flower skip_sw src_mac 00:02:00:00:00:01/44 action drop
The current flow offload infrastructure used does not support wildcard
matching for ethernet headers, so do not allow the second or third
commands to succeed. If a user wants to drop traffic on that interface
the protocol and MAC addresses need to be specified explicitly:
$ tc qdisc add dev p1p1 ingress
$ tc filter add dev p1p1 parent ffff: protocol arp \
flower skip_sw action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
flower skip_sw action drop
...
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
flower skip_sw src_mac 00:02:00:00:00:01 action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
flower skip_sw src_mac 00:02:00:00:00:02 action drop
...
There are also checks for VLAN parameters in this patch as other callers
may wildcard those parameters even if tc does not. Using different
flow infrastructure could allow this to work in the future for L2 flows,
but for now it does not.
Fixes: 2ae7408fedfe ("bnxt_en: bnxt: add TC flower filter offload support")
Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 59 ++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 65c2cee..ac193408 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -377,6 +377,30 @@ static bool is_wildcard(void *mask, int len)
return true;
}
+static bool is_exactmatch(void *mask, int len)
+{
+ const u8 *p = mask;
+ int i;
+
+ for (i = 0; i < len; i++)
+ if (p[i] != 0xff)
+ return false;
+
+ return true;
+}
+
+static bool bits_set(void *key, int len)
+{
+ const u8 *p = key;
+ int i;
+
+ for (i = 0; i < len; i++)
+ if (p[i] != 0)
+ return true;
+
+ return false;
+}
+
static int bnxt_hwrm_cfa_flow_alloc(struct bnxt *bp, struct bnxt_tc_flow *flow,
__le16 ref_flow_handle,
__le32 tunnel_handle, __le16 *flow_handle)
@@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
return false;
}
+ /* Currently source/dest MAC cannot be partial wildcard */
+ if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
+ !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
+ netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
+ return false;
+ }
+ if (bits_set(&flow->l2_key.dmac, sizeof(flow->l2_key.dmac)) &&
+ !is_exactmatch(&flow->l2_mask.dmac, sizeof(flow->l2_mask.dmac))) {
+ netdev_info(bp->dev, "Wildcard match unsupported for Dest MAC\n");
+ return false;
+ }
+
+ /* Currently VLAN fields cannot be partial wildcard */
+ if (bits_set(&flow->l2_key.inner_vlan_tci,
+ sizeof(flow->l2_key.inner_vlan_tci)) &&
+ !is_exactmatch(&flow->l2_mask.inner_vlan_tci,
+ sizeof(flow->l2_mask.inner_vlan_tci))) {
+ netdev_info(bp->dev, "Wildcard match unsupported for VLAN TCI\n");
+ return false;
+ }
+ if (bits_set(&flow->l2_key.inner_vlan_tpid,
+ sizeof(flow->l2_key.inner_vlan_tpid)) &&
+ !is_exactmatch(&flow->l2_mask.inner_vlan_tpid,
+ sizeof(flow->l2_mask.inner_vlan_tpid))) {
+ netdev_info(bp->dev, "Wildcard match unsupported for VLAN TPID\n");
+ return false;
+ }
+
+ /* Currently Ethertype must be set */
+ if (!is_exactmatch(&flow->l2_mask.ether_type,
+ sizeof(flow->l2_mask.ether_type))) {
+ netdev_info(bp->dev, "Wildcard match unsupported for Ethertype\n");
+ return false;
+ }
+
return true;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 1/6] bnxt_en: Fix ethtool -x crash when device is down.
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1523419093-18637-1-git-send-email-michael.chan@broadcom.com>
Fix ethtool .get_rxfh() crash by checking for valid indirection table
address before copying the data.
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 8d8ccd6..1f622ca 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -870,17 +870,22 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
u8 *hfunc)
{
struct bnxt *bp = netdev_priv(dev);
- struct bnxt_vnic_info *vnic = &bp->vnic_info[0];
+ struct bnxt_vnic_info *vnic;
int i = 0;
if (hfunc)
*hfunc = ETH_RSS_HASH_TOP;
- if (indir)
+ if (!bp->vnic_info)
+ return 0;
+
+ vnic = &bp->vnic_info[0];
+ if (indir && vnic->rss_table) {
for (i = 0; i < HW_HASH_INDEX_SIZE; i++)
indir[i] = le16_to_cpu(vnic->rss_table[i]);
+ }
- if (key)
+ if (key && vnic->rss_hash_key)
memcpy(key, vnic->rss_hash_key, HW_HASH_KEY_SIZE);
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 0/6] bnxt_en: Fixes for net.
From: Michael Chan @ 2018-04-11 3:58 UTC (permalink / raw)
To: davem; +Cc: netdev
This bug fix series include NULL pointer fixes in ethtool -x code path
and in the error clean up path when freeing IRQs, a ring accounting bug
that missed rings used by the RDMA driver, and 3 bug fixes related to TC
Flower and VF-reps.
Andy Gospodarek (1):
bnxt_en: do not allow wildcard matches for L2 flows
Michael Chan (3):
bnxt_en: Fix ethtool -x crash when device is down.
bnxt_en: Need to include RDMA rings in bnxt_check_rings().
bnxt_en: Fix NULL pointer dereference at bnxt_free_irq().
Sriharsha Basavapatna (2):
bnxt_en: Ignore src port field in decap filter nodes
bnxt_en: Support max-mtu with VF-reps
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 11 ++--
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 63 ++++++++++++++++++++++-
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 30 +++++++++++
4 files changed, 103 insertions(+), 5 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH 2/2] isdn: hisax_fcpcipnp: Replace mdelay with usleep_range in fcpcipnp_setup
From: Jia-Ju Bai @ 2018-04-11 3:33 UTC (permalink / raw)
To: isdn, davem, stephen, johannes.berg, arvind.yadav.cs
Cc: netdev, linux-kernel, Jia-Ju Bai
fcpcipnp_setup() is never called in atomic context.
The call chain ending up at fcpcipnp_setup() is:
[1] fcpcipnp_setup() <- fcpnp_probe()
fcpnp_probe() is set as ".probe" in struct pnp_driver.
This function is not called in atomic context.
Despite never getting called from atomic context, fcpcipnp_setup()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/isdn/hisax/hisax_fcpcipnp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/hisax/hisax_fcpcipnp.c b/drivers/isdn/hisax/hisax_fcpcipnp.c
index e4f7573..06068a42 100644
--- a/drivers/isdn/hisax/hisax_fcpcipnp.c
+++ b/drivers/isdn/hisax/hisax_fcpcipnp.c
@@ -772,11 +772,11 @@ static int fcpcipnp_setup(struct fritz_adapter *adapter)
// Reset
outb(0, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(10000, 11000);
outb(AVM_STATUS0_RESET, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(10000, 11000);
outb(0, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(10000, 11000);
switch (adapter->type) {
case AVM_FRITZ_PCIV2:
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] isdn: hisax_fcpcipnp: Replace mdelay with usleep_range in fcpci_init
From: Jia-Ju Bai @ 2018-04-11 3:33 UTC (permalink / raw)
To: isdn, davem, johannes.berg, arvind.yadav.cs, stephen
Cc: netdev, linux-kernel, Jia-Ju Bai
fcpci_init() is never called in atomic context.
The call chain ending up at fcpci_init() is:
[1] fcpci_init() <- fcpcipnp_setup() <- fcpnp_probe()
fcpnp_probe() is set as ".probe" in struct pnp_driver.
This function is not called in atomic context.
Despite never getting called from atomic context, fcpci_init()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/isdn/hisax/hisax_fcpcipnp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/hisax/hisax_fcpcipnp.c b/drivers/isdn/hisax/hisax_fcpcipnp.c
index e4f7573..4789c9d 100644
--- a/drivers/isdn/hisax/hisax_fcpcipnp.c
+++ b/drivers/isdn/hisax/hisax_fcpcipnp.c
@@ -706,7 +706,7 @@ static inline void fcpci_init(struct fritz_adapter *adapter)
outb(AVM_STATUS1_ENA_IOM | adapter->irq,
adapter->io + AVM_STATUS1);
- mdelay(10);
+ usleep_range(10000, 11000);
}
// ----------------------------------------------------------------------
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 0/2] vhost: fix vhost_vq_access_ok() log check
From: Jason Wang @ 2018-04-11 3:33 UTC (permalink / raw)
To: Stefan Hajnoczi, virtualization
Cc: Linus Torvalds, mst, netdev, syzkaller-bugs, kvm, linux-kernel
In-Reply-To: <20180411023541.15776-1-stefanha@redhat.com>
On 2018年04月11日 10:35, Stefan Hajnoczi wrote:
> v3:
> * Rebased onto net/master and resolved conflict [DaveM]
>
> v2:
> * Rewrote the conditional to make the vq access check clearer [Linus]
> * Added Patch 2 to make the return type consistent and harder to misuse [Linus]
>
> The first patch fixes the vhost virtqueue access check which was recently
> broken. The second patch replaces the int return type with bool to prevent
> future bugs.
>
> Stefan Hajnoczi (2):
> vhost: fix vhost_vq_access_ok() log check
> vhost: return bool from *_access_ok() functions
>
> drivers/vhost/vhost.h | 4 +--
> drivers/vhost/vhost.c | 70 ++++++++++++++++++++++++++-------------------------
> 2 files changed, 38 insertions(+), 36 deletions(-)
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply
* [PATCH v3 2/2] vhost: return bool from *_access_ok() functions
From: Stefan Hajnoczi @ 2018-04-11 2:35 UTC (permalink / raw)
To: virtualization
Cc: Linus Torvalds, jasowang, mst, netdev, syzkaller-bugs, kvm,
linux-kernel, Stefan Hajnoczi
In-Reply-To: <20180411023541.15776-1-stefanha@redhat.com>
Currently vhost *_access_ok() functions return int. This is error-prone
because there are two popular conventions:
1. 0 means failure, 1 means success
2. -errno means failure, 0 means success
Although vhost mostly uses #1, it does not do so consistently.
umem_access_ok() uses #2.
This patch changes the return type from int to bool so that false means
failure and true means success. This eliminates a potential source of
errors.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
drivers/vhost/vhost.h | 4 ++--
drivers/vhost/vhost.c | 66 +++++++++++++++++++++++++--------------------------
2 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d8ee85ae8fdc..6c844b90a168 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp);
-int vhost_vq_access_ok(struct vhost_virtqueue *vq);
-int vhost_log_access_ok(struct vhost_dev *);
+bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
+bool vhost_log_access_ok(struct vhost_dev *);
int vhost_get_vq_desc(struct vhost_virtqueue *,
struct iovec iov[], unsigned int iov_count,
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fc805b7fad9d..0fcb51a9940c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
-static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
+static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
{
u64 a = addr / VHOST_PAGE_SIZE / 8;
/* Make sure 64 bit math will not overflow. */
if (a > ULONG_MAX - (unsigned long)log_base ||
a + (unsigned long)log_base > ULONG_MAX)
- return 0;
+ return false;
return access_ok(VERIFY_WRITE, log_base + a,
(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
@@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
}
/* Caller should have vq mutex and device mutex. */
-static int vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
- int log_all)
+static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
+ int log_all)
{
struct vhost_umem_node *node;
if (!umem)
- return 0;
+ return false;
list_for_each_entry(node, &umem->umem_list, link) {
unsigned long a = node->userspace_addr;
if (vhost_overflow(node->userspace_addr, node->size))
- return 0;
+ return false;
if (!access_ok(VERIFY_WRITE, (void __user *)a,
node->size))
- return 0;
+ return false;
else if (log_all && !log_access_ok(log_base,
node->start,
node->size))
- return 0;
+ return false;
}
- return 1;
+ return true;
}
static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
@@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
/* Can we switch to this memory table? */
/* Caller should have device mutex but not vq mutex */
-static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
- int log_all)
+static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
+ int log_all)
{
int i;
for (i = 0; i < d->nvqs; ++i) {
- int ok;
+ bool ok;
bool log;
mutex_lock(&d->vqs[i]->mutex);
@@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
ok = vq_memory_access_ok(d->vqs[i]->log_base,
umem, log);
else
- ok = 1;
+ ok = true;
mutex_unlock(&d->vqs[i]->mutex);
if (!ok)
- return 0;
+ return false;
}
- return 1;
+ return true;
}
static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
@@ -959,21 +959,21 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
spin_unlock(&d->iotlb_lock);
}
-static int umem_access_ok(u64 uaddr, u64 size, int access)
+static bool umem_access_ok(u64 uaddr, u64 size, int access)
{
unsigned long a = uaddr;
/* Make sure 64 bit math will not overflow. */
if (vhost_overflow(uaddr, size))
- return -EFAULT;
+ return false;
if ((access & VHOST_ACCESS_RO) &&
!access_ok(VERIFY_READ, (void __user *)a, size))
- return -EFAULT;
+ return false;
if ((access & VHOST_ACCESS_WO) &&
!access_ok(VERIFY_WRITE, (void __user *)a, size))
- return -EFAULT;
- return 0;
+ return false;
+ return true;
}
static int vhost_process_iotlb_msg(struct vhost_dev *dev,
@@ -988,7 +988,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
ret = -EFAULT;
break;
}
- if (umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
+ if (!umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
ret = -EFAULT;
break;
}
@@ -1135,10 +1135,10 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
return 0;
}
-static int vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
- struct vring_desc __user *desc,
- struct vring_avail __user *avail,
- struct vring_used __user *used)
+static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
+ struct vring_desc __user *desc,
+ struct vring_avail __user *avail,
+ struct vring_used __user *used)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
@@ -1161,8 +1161,8 @@ static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
vq->meta_iotlb[type] = node;
}
-static int iotlb_access_ok(struct vhost_virtqueue *vq,
- int access, u64 addr, u64 len, int type)
+static bool iotlb_access_ok(struct vhost_virtqueue *vq,
+ int access, u64 addr, u64 len, int type)
{
const struct vhost_umem_node *node;
struct vhost_umem *umem = vq->iotlb;
@@ -1220,7 +1220,7 @@ EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
-int vhost_log_access_ok(struct vhost_dev *dev)
+bool vhost_log_access_ok(struct vhost_dev *dev)
{
return memory_access_ok(dev, dev->umem, 1);
}
@@ -1228,8 +1228,8 @@ EXPORT_SYMBOL_GPL(vhost_log_access_ok);
/* Verify access for write logging. */
/* Caller should have vq mutex and device mutex */
-static int vq_log_access_ok(struct vhost_virtqueue *vq,
- void __user *log_base)
+static bool vq_log_access_ok(struct vhost_virtqueue *vq,
+ void __user *log_base)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
@@ -1242,14 +1242,14 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
/* Can we start vq? */
/* Caller should have vq mutex and device mutex */
-int vhost_vq_access_ok(struct vhost_virtqueue *vq)
+bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
if (!vq_log_access_ok(vq, vq->log_base))
- return 0;
+ return false;
/* Access validation occurs at prefetch time with IOTLB */
if (vq->iotlb)
- return 1;
+ return true;
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
}
--
2.14.3
^ permalink raw reply related
* [PATCH v3 1/2] vhost: fix vhost_vq_access_ok() log check
From: Stefan Hajnoczi @ 2018-04-11 2:35 UTC (permalink / raw)
To: virtualization
Cc: Linus Torvalds, jasowang, mst, netdev, syzkaller-bugs, kvm,
linux-kernel, Stefan Hajnoczi
In-Reply-To: <20180411023541.15776-1-stefanha@redhat.com>
Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
when IOTLB is enabled") introduced a regression. The logic was
originally:
if (vq->iotlb)
return 1;
return A && B;
After the patch the short-circuit logic for A was inverted:
if (A || vq->iotlb)
return A;
return B;
This patch fixes the regression by rewriting the checks in the obvious
way, no longer returning A when vq->iotlb is non-NULL (which is hard to
understand).
Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
drivers/vhost/vhost.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index bec722e41f58..fc805b7fad9d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1244,10 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
/* Caller should have vq mutex and device mutex */
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
- int ret = vq_log_access_ok(vq, vq->log_base);
+ if (!vq_log_access_ok(vq, vq->log_base))
+ return 0;
- if (ret || vq->iotlb)
- return ret;
+ /* Access validation occurs at prefetch time with IOTLB */
+ if (vq->iotlb)
+ return 1;
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
}
--
2.14.3
^ permalink raw reply related
* [PATCH v3 0/2] vhost: fix vhost_vq_access_ok() log check
From: Stefan Hajnoczi @ 2018-04-11 2:35 UTC (permalink / raw)
To: virtualization
Cc: Linus Torvalds, jasowang, mst, netdev, syzkaller-bugs, kvm,
linux-kernel, Stefan Hajnoczi
v3:
* Rebased onto net/master and resolved conflict [DaveM]
v2:
* Rewrote the conditional to make the vq access check clearer [Linus]
* Added Patch 2 to make the return type consistent and harder to misuse [Linus]
The first patch fixes the vhost virtqueue access check which was recently
broken. The second patch replaces the int return type with bool to prevent
future bugs.
Stefan Hajnoczi (2):
vhost: fix vhost_vq_access_ok() log check
vhost: return bool from *_access_ok() functions
drivers/vhost/vhost.h | 4 +--
drivers/vhost/vhost.c | 70 ++++++++++++++++++++++++++-------------------------
2 files changed, 38 insertions(+), 36 deletions(-)
--
2.14.3
^ permalink raw reply
* [PATCH] net: sun: cassini: Replace GFP_ATOMIC with GFP_KERNEL in cas_check_invariants
From: Jia-Ju Bai @ 2018-04-11 2:34 UTC (permalink / raw)
To: asun; +Cc: netdev, linux-kernel, Jia-Ju Bai
cas_check_invariants() is never called in atomic context.
cas_check_invariants() is only called by cas_init_one(), which is
only set as ".probe" in struct pci_driver.
Despite never getting called from atomic context,
cas_check_invariants() calls alloc_pages() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of sucessful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/net/ethernet/sun/cassini.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sun/cassini.c b/drivers/net/ethernet/sun/cassini.c
index 382993c..4dd38e3 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -3412,7 +3412,7 @@ static int cas_check_invariants(struct cas *cp)
#ifdef USE_PAGE_ORDER
if (PAGE_SHIFT < CAS_JUMBO_PAGE_SHIFT) {
/* see if we can allocate larger pages */
- struct page *page = alloc_pages(GFP_ATOMIC,
+ struct page *page = alloc_pages(GFP_KERNEL,
CAS_JUMBO_PAGE_SHIFT -
PAGE_SHIFT);
if (page) {
--
1.9.1
^ permalink raw reply related
* [PATCH] net: samsung: sxgbe: Replace mdelay with usleep_range in sxgbe_sw_reset
From: Jia-Ju Bai @ 2018-04-11 2:21 UTC (permalink / raw)
To: bh74.an, ks.giri, vipul.pandya; +Cc: netdev, linux-kernel, Jia-Ju Bai
sxgbe_sw_reset() is never called in atomic context.
sxgbe_sw_reset() is only called by sxgbe_drv_probe(), which is
only called by sxgbe_platform_probe().
sxgbe_platform_probe() is set as ".probe" in struct platform_driver.
This function is not called in atomic context.
Despite never getting called from atomic context, sxgbe_sw_reset()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 89831ad..99cd586 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -2038,7 +2038,7 @@ static int sxgbe_sw_reset(void __iomem *addr)
if (!(readl(addr + SXGBE_DMA_MODE_REG) &
SXGBE_DMA_SOFT_RESET))
break;
- mdelay(10);
+ usleep(10000, 11000);
}
if (retry_count < 0)
--
1.9.1
^ permalink raw reply related
* Re: Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Jason Wang @ 2018-04-11 2:18 UTC (permalink / raw)
To: Liang, Cunming, Michael S. Tsirkin
Cc: Paolo Bonzini, Bie, Tiwei, alex.williamson@redhat.com,
ddutile@redhat.com, Duyck, Alexander H,
virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org, Daly, Dan, Wang, Zhihong, Tan, Jianfeng,
Wang, Xiao W
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9226AF@SHSMSX104.ccr.corp.intel.com>
On 2018年04月10日 22:23, Liang, Cunming wrote:
>> -----Original Message-----
>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>> Sent: Tuesday, April 10, 2018 9:36 PM
>> To: Liang, Cunming<cunming.liang@intel.com>
>> Cc: Paolo Bonzini<pbonzini@redhat.com>; Bie, Tiwei<tiwei.bie@intel.com>;
>> Jason Wang<jasowang@redhat.com>;alex.williamson@redhat.com;
>> ddutile@redhat.com; Duyck, Alexander H<alexander.h.duyck@intel.com>;
>> virtio-dev@lists.oasis-open.org;linux-kernel@vger.kernel.org;
>> kvm@vger.kernel.org;virtualization@lists.linux-foundation.org;
>> netdev@vger.kernel.org; Daly, Dan<dan.daly@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>; Tan, Jianfeng<jianfeng.tan@intel.com>; Wang, Xiao
>> W<xiao.w.wang@intel.com>
>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost
>> backend
>>
>> On Tue, Apr 10, 2018 at 09:23:53AM +0000, Liang, Cunming wrote:
>>>> -----Original Message-----
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> Sent: Tuesday, April 10, 2018 3:52 PM
>>>> To: Bie, Tiwei<tiwei.bie@intel.com>; Jason Wang
>>>> <jasowang@redhat.com>
>>>> Cc:mst@redhat.com;alex.williamson@redhat.com;ddutile@redhat.com;
>>>> Duyck, Alexander H<alexander.h.duyck@intel.com>;
>>>> virtio-dev@lists.oasis- open.org;linux-kernel@vger.kernel.org;
>>>> kvm@vger.kernel.org;virtualization@lists.linux-foundation.org;
>>>> netdev@vger.kernel.org; Daly, Dan<dan.daly@intel.com>; Liang,
>>>> Cunming<cunming.liang@intel.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; Tan, Jianfeng<jianfeng.tan@intel.com>;
>>>> Wang, Xiao W<xiao.w.wang@intel.com>
>>>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based
>>>> hardware vhost backend
>>>>
>>>> On 10/04/2018 06:57, Tiwei Bie wrote:
>>>>>> So you just move the abstraction layer from qemu to kernel, and
>>>>>> you still need different drivers in kernel for different device
>>>>>> interfaces of accelerators. This looks even more complex than
>>>>>> leaving it in qemu. As you said, another idea is to implement
>>>>>> userspace vhost backend for accelerators which seems easier and
>>>>>> could co-work with other parts of qemu without inventing new type of
>> messages.
>>>>> I'm not quite sure. Do you think it's acceptable to add various
>>>>> vendor specific hardware drivers in QEMU?
>>>> I think so. We have vendor-specific quirks, and at some point there
>>>> was an idea of using quirks to implement (vendor-specific) live
>>>> migration support for assigned devices.
>>> Vendor-specific quirks of accessing VGA is a small portion. Other major portions
>> are still handled by guest driver.
>>> While in this case, when saying various vendor specific drivers in QEMU, it says
>> QEMU takes over and provides the entire user space device drivers. Some parts
>> are even not relevant to vhost, they're basic device function enabling. Moreover,
>> it could be different kinds of devices(network/block/...) under vhost. No matter
>> # of vendors or # of types, total LOC is not small.
>>> The idea is to avoid introducing these extra complexity out of QEMU, keeping
>> vhost adapter simple. As vhost protocol is de factor standard, it leverages kernel
>> device driver to provide the diversity. Changing once in QEMU, then it supports
>> multi-vendor devices whose drivers naturally providing kernel driver there.
>>> If QEMU is going to build a user space driver framework there, we're open mind
>> on that, even leveraging DPDK as the underlay library. Looking forward to more
>> others' comments from community.
>>> Steve
>> Dependency on a kernel driver is fine IMHO. It's the dependency on a DPDK
>> backend that makes people unhappy, since the functionality in question is setup-
>> time only.
> Agreed, we don't see dependency on kernel driver is a problem.
At engineering level, kernel driver is harder than userspace driver.
>
> mdev based vhost backend (this patch set) is independent with vhost-user extension patch set. In fact, there're a few vhost-user providers, DPDK librte_vhost is one of them. FD.IO/VPP and snabbswitch have their own vhost-user providers. So I can't agree on vhost-user extension patch depends on DPDK backend. But anyway, that's the topic of another mail thread.
>
Well we can treat mdev as another kind of transport of vhost-user. And
technically we can even implement a relay mdev than forward vhost-user
messages to dpdk.
Thanks
^ permalink raw reply
* [PATCH] net: ieee802154: atusb: Replace GFP_ATOMIC with GFP_KERNEL in atusb_probe
From: Jia-Ju Bai @ 2018-04-11 2:14 UTC (permalink / raw)
To: stefan, alex.aring; +Cc: linux-wpan, netdev, linux-kernel, Jia-Ju Bai
atusb_probe() is never called in atomic context.
This function is only set as ".probe" in struct usb_driver.
Despite never getting called from atomic context,
atusb_probe() calls usb_alloc_urb() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of sucessful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/net/ieee802154/atusb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c
index ef68851..ab6a505 100644
--- a/drivers/net/ieee802154/atusb.c
+++ b/drivers/net/ieee802154/atusb.c
@@ -789,7 +789,7 @@ static int atusb_probe(struct usb_interface *interface,
atusb->tx_dr.bRequest = ATUSB_TX;
atusb->tx_dr.wValue = cpu_to_le16(0);
- atusb->tx_urb = usb_alloc_urb(0, GFP_ATOMIC);
+ atusb->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!atusb->tx_urb)
goto fail;
--
1.9.1
^ permalink raw reply related
* Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Jason Wang @ 2018-04-11 2:08 UTC (permalink / raw)
To: Liang, Cunming, Paolo Bonzini, Bie, Tiwei
Cc: mst@redhat.com, alex.williamson@redhat.com, ddutile@redhat.com,
Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
Daly, Dan, Wang, Zhihong, Tan, Jianfeng, Wang, Xiao W
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9222F4@SHSMSX104.ccr.corp.intel.com>
On 2018年04月10日 17:23, Liang, Cunming wrote:
>
>> -----Original Message-----
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Sent: Tuesday, April 10, 2018 3:52 PM
>> To: Bie, Tiwei <tiwei.bie@intel.com>; Jason Wang <jasowang@redhat.com>
>> Cc: mst@redhat.com; alex.williamson@redhat.com; ddutile@redhat.com;
>> Duyck, Alexander H <alexander.h.duyck@intel.com>; virtio-dev@lists.oasis-
>> open.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
>> virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Daly, Dan
>> <dan.daly@intel.com>; Liang, Cunming <cunming.liang@intel.com>; Wang,
>> Zhihong <zhihong.wang@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>;
>> Wang, Xiao W <xiao.w.wang@intel.com>
>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware
>> vhost backend
>>
>> On 10/04/2018 06:57, Tiwei Bie wrote:
>>>> So you just move the abstraction layer from qemu to kernel, and you
>>>> still need different drivers in kernel for different device
>>>> interfaces of accelerators. This looks even more complex than leaving
>>>> it in qemu. As you said, another idea is to implement userspace vhost
>>>> backend for accelerators which seems easier and could co-work with
>>>> other parts of qemu without inventing new type of messages.
>>> I'm not quite sure. Do you think it's acceptable to add various vendor
>>> specific hardware drivers in QEMU?
>> I think so. We have vendor-specific quirks, and at some point there was an
>> idea of using quirks to implement (vendor-specific) live migration support for
>> assigned devices.
> Vendor-specific quirks of accessing VGA is a small portion. Other major portions are still handled by guest driver.
>
> While in this case, when saying various vendor specific drivers in QEMU, it says QEMU takes over and provides the entire user space device drivers. Some parts are even not relevant to vhost, they're basic device function enabling. Moreover, it could be different kinds of devices(network/block/...) under vhost. No matter # of vendors or # of types, total LOC is not small.
>
> The idea is to avoid introducing these extra complexity out of QEMU, keeping vhost adapter simple. As vhost protocol is de factor standard, it leverages kernel device driver to provide the diversity. Changing once in QEMU, then it supports multi-vendor devices whose drivers naturally providing kernel driver there.
Let me clarify my question, it's not qemu vs kenrel but userspace vs
kernel. It could be a library which could be linked to qemu. Doing it in
userspace have the following obvious advantages:
- attack surface was limited to userspace
- easier to be maintained (compared to kernel driver)
- easier to be extended without introducing new userspace/kernel interfaces
- not tied to a specific operating system
If we want to do it in the kernel, need to consider to unify code
between mdev device driver and generic driver. For net driver, maybe we
can even consider to do it on top of exist drivers.
>
> If QEMU is going to build a user space driver framework there, we're open mind on that, even leveraging DPDK as the underlay library. Looking forward to more others' comments from community.
I'm doing this now by implementing vhost inside qemu IOThreads. Hope I
can post RFC in few months.
Thanks
> Steve
>
>> Paolo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox