* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Kirill A. Shutemov @ 2014-12-08 16:46 UTC (permalink / raw)
To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141204202011.GO29748@ZenIV.linux.org.uk>
On Thu, Dec 04, 2014 at 08:20:11PM +0000, Al Viro wrote:
> First of all, I want to apologize for the nastiness of preprocessor
> use in this series. Seeing that the whole "macros that look like new kinds
> of C statements" thing (including list_for_each_...(), etc) is very much not
> to my liking, I really don't trust my taste on finer details and I'd very
> much like some feedback.
>
> The reason for doing that kind of tricks is that iov_iter.c keeps
> growing more and more boilerplate code. For iov_iter-net series we need
> * csum_and_copy_from_iter()
> * csum_and_copy_to_iter()
> * copy_from_iter_nocache()
> That's 3 new primitives, each in 2 variants (iovec and bvec).
> * ITER_KVEC handled without going through uaccess.h stuff (and
> independent of set_fs() state).
> And *that* means 3 variants intstead of 2 for most of the existing primitives.
> That's far too much, and the amount of copies of the same logics would pretty
> much guarantee that it will be a breeding ground for hard-to-kill bugs.
>
> The following series (also in vfs.git#iov_iter) actually manages to
> do all of the above *and* shrink the damn thing quite a bit. The generated
> code appears to be no worse than before. The price is a couple of iterator
> macros - iterate_all_kinds() and iterate_and_advance(). They are given an
> iov_iter, size (i.e. the amount of data in iov_iter beginning we want to go
> through), name of the loop variable and 3 variants of loop body - for iovec,
> bvec and kvec resp. Loop variable is declared *inside* the expansion of those
> suckers according to the kind of iov_iter - it's struct iovec, struct bio_vec
> or struct kvec, covering the current range to deal with.
> The difference between those two is that iterate_and_advance() will
> advance the iov_iter by the amount it has handled and iterate_all_kinds()
> will leave iov_iter unchanged.
>
> Unless I hear anybody yelling, it goes into vfs.git#for-next today,
> so if you have objections, suggestions, etc., give those *now*.
>
> Al Viro (13):
> iov_iter.c: macros for iterating over iov_iter
> iov_iter.c: iterate_and_advance
> iov_iter.c: convert iov_iter_npages() to iterate_all_kinds
> iov_iter.c: convert iov_iter_get_pages() to iterate_all_kinds
> iov_iter.c: convert iov_iter_get_pages_alloc() to iterate_all_kinds
> iov_iter.c: convert iov_iter_zero() to iterate_and_advance
> iov_iter.c: get rid of bvec_copy_page_{to,from}_iter()
> iov_iter.c: convert copy_from_iter() to iterate_and_advance
> iov_iter.c: convert copy_to_iter() to iterate_and_advance
> iov_iter.c: handle ITER_KVEC directly
> csum_and_copy_..._iter()
> new helper: iov_iter_kvec()
> copy_from_iter_nocache()
I guess this crash is related to the patchset.
[ 102.337742] ------------[ cut here ]------------
[ 102.338270] kernel BUG at /home/kas/git/public/linux-next/arch/x86/mm/physaddr.c:26!
[ 102.339043] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 102.339622] Modules linked in:
[ 102.339951] CPU: 2 PID: 6029 Comm: trinity-c23 Not tainted 3.18.0-next-20141208-00036-gc7edb4791544-dirty #269
[ 102.340011] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
[ 102.340011] task: ffff880041c51510 ti: ffff880049c70000 task.ti: ffff880049c70000
[ 102.340011] RIP: 0010:[<ffffffff8104d8c0>] [<ffffffff8104d8c0>] __phys_addr+0x40/0x60
[ 102.340011] RSP: 0018:ffff880049c73838 EFLAGS: 00010206
[ 102.340011] RAX: 00004100174b4000 RBX: ffff880049c73b08 RCX: 0000000000000028
[ 102.340011] RDX: 0000000000000041 RSI: ffff88015dc980a8 RDI: ffffc900174b4000
[ 102.340011] RBP: ffff880049c73838 R08: ffffc900174b4000 R09: 000000000000000c
[ 102.340011] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88015dc980a8
[ 102.340011] R13: ffffc900174f4000 R14: ffffea0000000000 R15: ffffc900174b4000
[ 102.340011] FS: 00007fcdd37fb700(0000) GS:ffff88017aa00000(0000) knlGS:0000000000000000
[ 102.340011] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 102.340011] CR2: 00000000006246a8 CR3: 0000000068dbb000 CR4: 00000000001406e0
[ 102.340011] Stack:
[ 102.340011] ffff880049c73888 ffffffff8117a96b 0000000000000002 0000000000040000
[ 102.340011] ffffffff81204b3f ffff88015dc98028 0000000000000000 ffff880049c73a78
[ 102.340011] ffff88015dc98000 ffff880049c73a10 ffff880049c73978 ffffffff81203ec9
[ 102.340011] Call Trace:
[ 102.340011] [<ffffffff8117a96b>] iov_iter_get_pages+0x17b/0x390
[ 102.340011] [<ffffffff81204b3f>] ? __blockdev_direct_IO+0x15f/0x16a0
[ 102.340011] [<ffffffff81203ec9>] do_direct_IO+0x10a9/0x1bc0
[ 102.340011] [<ffffffff810a92d8>] ? lockdep_init_map+0x68/0x5c0
[ 102.340011] [<ffffffff81204d6c>] __blockdev_direct_IO+0x38c/0x16a0
[ 102.340011] [<ffffffff810a9d27>] ? __lock_acquire+0x4f7/0xd40
[ 102.340011] [<ffffffff810a73f9>] ? mark_held_locks+0x79/0xb0
[ 102.340011] [<ffffffff8133b510>] ? xfs_get_blocks+0x20/0x20
[ 102.340011] [<ffffffff81338ff0>] xfs_vm_direct_IO+0x120/0x140
[ 102.340011] [<ffffffff8133b510>] ? xfs_get_blocks+0x20/0x20
[ 102.340011] [<ffffffff810aa773>] ? lock_release_non_nested+0x203/0x3d0
[ 102.340011] [<ffffffff8135b9a7>] ? xfs_ilock+0x167/0x2e0
[ 102.340011] [<ffffffff8114d957>] generic_file_read_iter+0x517/0x6a0
[ 102.340011] [<ffffffff810a73f9>] ? mark_held_locks+0x79/0xb0
[ 102.340011] [<ffffffff8185e192>] ? __mutex_unlock_slowpath+0xb2/0x190
[ 102.340011] [<ffffffff8134bc2f>] xfs_file_read_iter+0x12f/0x460
[ 102.340011] [<ffffffff811c237e>] new_sync_read+0x7e/0xb0
[ 102.340011] [<ffffffff811c3528>] __vfs_read+0x18/0x50
[ 102.340011] [<ffffffff811c35ed>] vfs_read+0x8d/0x150
[ 102.340011] [<ffffffff811c89e8>] kernel_read+0x48/0x60
[ 102.340011] [<ffffffff810f4a92>] copy_module_from_fd.isra.51+0x112/0x170
[ 102.340011] [<ffffffff810f7646>] SyS_finit_module+0x56/0x80
[ 102.340011] [<ffffffff81861f92>] system_call_fastpath+0x12/0x17
[ 102.340011] Code: 00 00 00 00 00 78 00 00 48 01 f8 48 39 c2 72 1b 0f b6 0d 9d 7a ec 00 48 89 c2 48 d3 ea 48 85 d2 75 09 5d c3 0f 1f 80 00 00 00 00 <0f> 0b 66 0f 1f 44 00 00 48 89 d0 48 03 05 3e 87 dc 00 48 81 fa
[ 102.340011] RIP [<ffffffff8104d8c0>] __phys_addr+0x40/0x60
[ 102.340011] RSP <ffff880049c73838>
[ 102.371939] ---[ end trace e07368268cd6b49c ]---
--
Kirill A. Shutemov
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Pali Rohár @ 2014-12-08 16:47 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Ming Lei, Pavel Machek, John W. Linville, Grazvydas Ignotas,
linux-wireless@vger.kernel.org, Network Development,
Linux Kernel Mailing List, Ivaylo Dimitrov, Aaro Koskinen,
Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <20141208163714.GA31169@kroah.com>
[-- Attachment #1: Type: Text/Plain, Size: 1991 bytes --]
On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
<pali.rohar@gmail.com> wrote:
> > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> > > /**
> > >
> > > + * request_firmware_prefer_user: - prefer usermode helper
> > > for loading firmware + * @firmware_p: pointer to firmware
> > > image
> > > + * @name: name of firmware file
> > > + * @device: device for which firmware is being loaded
> > > + *
> > > + * This function works pretty much like
> > > request_firmware(), but it prefer + * usermode helper. If
> > > usermode helper fails then it fallback to direct access.
> > > + * Usefull for dynamic or model specific firmware data.
> > > + **/
> > > +int request_firmware_prefer_user(const struct firmware
> > > **firmware_p, + const char
> > > *name, struct device *device) +{
> > > + int ret;
> > > + __module_get(THIS_MODULE);
> > > + ret = _request_firmware(firmware_p, name, device,
> > > + FW_OPT_UEVENT |
> > > FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >
> > I'd like to introduce request_firmware_user() which only
> > requests firmware from user space, and this way is simpler
> > and more flexible since we have request_firmware_direct()
> > already.
>
> Why would a driver care about what program provides the
> firmware? It shouldn't at all, and we want to get rid of the
> userspace firmware loader, not encourage drivers to use it
> "exclusively" at all.
>
Do not remove it! Without userspace firmware loader it is
impossible to load dynamic firmware files.
> So no, I don't want to see this, and I don't want drivers to
> worry about this either.
>
> greg k-h
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH][net-next] net: avoid to call skb_queue_len again
From: Sergei Shtylyov @ 2014-12-08 16:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Li RongQing, netdev
In-Reply-To: <1418051436.15618.43.camel@edumazet-glaptop2.roam.corp.google.com>
Hello.
On 12/08/2014 06:10 PM, Eric Dumazet wrote:
>> I expect you to also refine the description, so that it's meaningful,
>> unlike now.
> It seems obvious to me Li is not a native English speaker.
Me neither. :-)
However, the good command of English language was a requirement when I was
first hired to do the Linux development.
> I understood
> the patch very well, and the changelog seemed fine to me.
Oh, if you say so...
> What about you provide this description instead, since you seem to care
> very much ?
I mostly care for others; I don't suppose much people except you are able
to understand the current variant. And I now have neither enough time nor
enough understanding to write a proper description for this patch. I can only
suggest that you refine the description for others if you can understand it so
well.
> Thanks !
Not at all.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Hannes Frederic Sowa @ 2014-12-08 16:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Herbert Xu, Thomas Graf
In-Reply-To: <1418056230-8700-1-git-send-email-dborkman@redhat.com>
On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
> For netlink, we shouldn't be using arch_fast_hash() as a hashing
> discipline, but rather jhash() instead.
>
> Since netlink sockets can be opened by any user, a local attacker
> would be able to easily create collisions with the DPDK-derived
> arch_fast_hash(), which trades off performance for security by
> using crc32 CPU instructions on x86_64.
>
> While it might have a legimite use case in other places, it should
> be avoided in netlink context, though. As rhashtable's API is very
> flexible, we could later on still decide on other hashing disciplines,
> if legitimate.
>
> Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
> Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> net/netlink/af_netlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 0007b81..b6bf8e8 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -3130,7 +3130,7 @@ static int __init netlink_proto_init(void)
> .head_offset = offsetof(struct netlink_sock, node),
> .key_offset = offsetof(struct netlink_sock, portid),
> .key_len = sizeof(u32), /* portid */
> - .hashfn = arch_fast_hash,
> + .hashfn = jhash,
> .max_shift = 16, /* 64K */
> .grow_decision = rht_grow_above_75,
> .shrink_decision = rht_shrink_below_30,
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
In net-next, some time soon, we should try to let all function pointers
to jhash() use one non-inline version. The other arch_fast_hash patch
adds __jhash for x86-only, we can move it over to lib/.
Thanks,
Hannes
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Marcel Holtmann @ 2014-12-08 17:05 UTC (permalink / raw)
To: Pali Rohár
Cc: Greg Kroah-Hartman, Ming Lei, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <201412081747.30965@pali>
Hi Pali,
>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>>>> /**
>>>>
>>>> + * request_firmware_prefer_user: - prefer usermode helper
>>>> for loading firmware + * @firmware_p: pointer to firmware
>>>> image
>>>> + * @name: name of firmware file
>>>> + * @device: device for which firmware is being loaded
>>>> + *
>>>> + * This function works pretty much like
>>>> request_firmware(), but it prefer + * usermode helper. If
>>>> usermode helper fails then it fallback to direct access.
>>>> + * Usefull for dynamic or model specific firmware data.
>>>> + **/
>>>> +int request_firmware_prefer_user(const struct firmware
>>>> **firmware_p, + const char
>>>> *name, struct device *device) +{
>>>> + int ret;
>>>> + __module_get(THIS_MODULE);
>>>> + ret = _request_firmware(firmware_p, name, device,
>>>> + FW_OPT_UEVENT |
>>>> FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>
>>> I'd like to introduce request_firmware_user() which only
>>> requests firmware from user space, and this way is simpler
>>> and more flexible since we have request_firmware_direct()
>>> already.
>>
>> Why would a driver care about what program provides the
>> firmware? It shouldn't at all, and we want to get rid of the
>> userspace firmware loader, not encourage drivers to use it
>> "exclusively" at all.
>>
>
> Do not remove it! Without userspace firmware loader it is
> impossible to load dynamic firmware files.
why is this dynamic in the first place. It does not sound like dynamic data to me at all. This is like the WiFi MAC address(es) or Bluetooth BD_ADDR. They are all static information. The only difference is that they are on the host accessibly filesystem or storage and not on the device itself.
To be honest, for Bluetooth we solved this now. If the device is missing key information like the calibration data or BD_ADDR, then it comes up unconfigured. A userspace process can then go and load the right data into it and then the device becomes available as Bluetooth device.
Trying to use request_firmware to load some random data and insist on going through userspace helper for that sounds crazy to me. Especially since we are trying hard to get away from the userspace loader. Forcing to keep it for new stuff sounds backwards to me.
With the special Nokia partition in mind, why hasn't this been turned into a mountable filesystem or into a driver/subsystem that can access the data direct from the kernel. I advocated for this some time ago. Maybe there should be a special subsystem for access to these factory persistent information that drivers then just can access. I seem to remember that some systems provide these via ACPI. Why does the ARM platform has to be special here?
And the problem of getting Ethernet and WiFi MAC address and Bluetooth BD_ADDR comes up many many times. Why not have something generic here. And don't tell me request_firmware is that generic solution ;)
Regards
Marcel
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Pali Rohár @ 2014-12-08 17:11 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Greg Kroah-Hartman, Ming Lei, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <E5F3BB54-6365-4856-A231-A5FEDAEA217F@holtmann.org>
[-- Attachment #1: Type: Text/Plain, Size: 3623 bytes --]
On Monday 08 December 2014 18:05:37 Marcel Holtmann wrote:
> Hi Pali,
>
> >>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> >>>> /**
> >>>>
> >>>> + * request_firmware_prefer_user: - prefer usermode
> >>>> helper for loading firmware + * @firmware_p: pointer to
> >>>> firmware image
> >>>> + * @name: name of firmware file
> >>>> + * @device: device for which firmware is being loaded
> >>>> + *
> >>>> + * This function works pretty much like
> >>>> request_firmware(), but it prefer + * usermode helper. If
> >>>> usermode helper fails then it fallback to direct access.
> >>>> + * Usefull for dynamic or model specific firmware data.
> >>>> + **/
> >>>> +int request_firmware_prefer_user(const struct firmware
> >>>> **firmware_p, + const char
> >>>> *name, struct device *device) +{
> >>>> + int ret;
> >>>> + __module_get(THIS_MODULE);
> >>>> + ret = _request_firmware(firmware_p, name, device,
> >>>> + FW_OPT_UEVENT |
> >>>> FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
> >>>> + return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>>
> >>> I'd like to introduce request_firmware_user() which only
> >>> requests firmware from user space, and this way is simpler
> >>> and more flexible since we have request_firmware_direct()
> >>> already.
> >>
> >> Why would a driver care about what program provides the
> >> firmware? It shouldn't at all, and we want to get rid of
> >> the userspace firmware loader, not encourage drivers to
> >> use it "exclusively" at all.
> >
> > Do not remove it! Without userspace firmware loader it is
> > impossible to load dynamic firmware files.
>
> why is this dynamic in the first place. It does not sound like
> dynamic data to me at all. This is like the WiFi MAC
> address(es) or Bluetooth BD_ADDR. They are all static
> information. The only difference is that they are on the host
> accessibly filesystem or storage and not on the device
> itself.
>
> To be honest, for Bluetooth we solved this now. If the device
> is missing key information like the calibration data or
> BD_ADDR, then it comes up unconfigured. A userspace process
> can then go and load the right data into it and then the
> device becomes available as Bluetooth device.
>
> Trying to use request_firmware to load some random data and
> insist on going through userspace helper for that sounds
> crazy to me. Especially since we are trying hard to get away
> from the userspace loader. Forcing to keep it for new stuff
> sounds backwards to me.
>
> With the special Nokia partition in mind, why hasn't this been
> turned into a mountable filesystem or into a driver/subsystem
> that can access the data direct from the kernel. I advocated
> for this some time ago. Maybe there should be a special
> subsystem for access to these factory persistent information
> that drivers then just can access. I seem to remember that
> some systems provide these via ACPI. Why does the ARM
> platform has to be special here?
>
> And the problem of getting Ethernet and WiFi MAC address and
> Bluetooth BD_ADDR comes up many many times. Why not have
> something generic here. And don't tell me request_firmware is
> that generic solution ;)
>
> Regards
>
> Marcel
Hi Marcel. I think you did not understand this problem. This
discussion is not about mac address. Please read email thread
again and if there are some unclear pars, then ask. Thanks!
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [ovs-dev] OVS Kernel Datapath development
From: Thomas Graf @ 2014-12-08 17:15 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, dev@openvswitch.org
In-Reply-To: <CALnjE+rn_Giv+8TM1_E2faBftRkivXMYHcjn-4Eq4Gu15r=CiQ@mail.gmail.com>
On 12/07/14 at 08:47pm, Pravin Shelar wrote:
> Since the beginning OVS kernel datapath development is primarily done
> on external OVS repo. Now we have mostly synced upstream and external
> OVS. So we have decided to change this process. New process is as
> follows.
>
> 1. OVS feature development that involves kernel datapath should be
> done on net-next tree datapath.
> 2. Such feature patch series should be posted on netdev and ovs-dev
> mailing list.
> 3. Once review is done for entire series, kernel and OVS userspace
> patches will be merged in respective repo.
> 4. After the merge developer is suppose to send patches for external
> kernel datapath along with old kernel compatibility code. So that we
> can keep external datapath insync.
+1
Just to be clear, by respective repo do you mean net-next/net or will
you maintain a net-next branch on git.kernel.org and continue doing
pull requests?
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Dave Taht @ 2014-12-08 17:20 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Daniel Borkmann, davem@davemloft.net, netdev@vger.kernel.org,
Herbert Xu, Thomas Graf
In-Reply-To: <1418057780.29477.12.camel@localhost>
On Mon, Dec 8, 2014 at 8:56 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
>> For netlink, we shouldn't be using arch_fast_hash() as a hashing
>> discipline, but rather jhash() instead.
I am not particularly happy with the amount of entropy in
static inline u32 ipv6_addr_hash(const struct in6_addr *a)
{
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
const unsigned long *ul = (const unsigned long *)a;
unsigned long x = ul[0] ^ ul[1];
return (u32)(x ^ (x >> 32));
#else
return (__force u32)(a->s6_addr32[0] ^ a->s6_addr32[1] ^
a->s6_addr32[2] ^ a->s6_addr32[3]);
#endif
}
is this worth improving somehow?
>> Since netlink sockets can be opened by any user, a local attacker
>> would be able to easily create collisions with the DPDK-derived
>> arch_fast_hash(), which trades off performance for security by
>> using crc32 CPU instructions on x86_64.
>>
>> While it might have a legimite use case in other places, it should
>> be avoided in netlink context, though. As rhashtable's API is very
>> flexible, we could later on still decide on other hashing disciplines,
>> if legitimate.
>>
>> Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
>> Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Thomas Graf <tgraf@suug.ch>
>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>> net/netlink/af_netlink.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>> index 0007b81..b6bf8e8 100644
>> --- a/net/netlink/af_netlink.c
>> +++ b/net/netlink/af_netlink.c
>> @@ -3130,7 +3130,7 @@ static int __init netlink_proto_init(void)
>> .head_offset = offsetof(struct netlink_sock, node),
>> .key_offset = offsetof(struct netlink_sock, portid),
>> .key_len = sizeof(u32), /* portid */
>> - .hashfn = arch_fast_hash,
>> + .hashfn = jhash,
>> .max_shift = 16, /* 64K */
>> .grow_decision = rht_grow_above_75,
>> .shrink_decision = rht_shrink_below_30,
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> In net-next, some time soon, we should try to let all function pointers
> to jhash() use one non-inline version. The other arch_fast_hash patch
> adds __jhash for x86-only, we can move it over to lib/.
>
> Thanks,
> Hannes
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dave Täht
http://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Hannes Frederic Sowa @ 2014-12-08 17:25 UTC (permalink / raw)
To: Dave Taht; +Cc: Daniel Borkmann, davem, netdev, Herbert Xu, Thomas Graf
In-Reply-To: <CAA93jw5bAqxbpxyOHEHd6XSEn4-HK0a9qw3Cb_Vecb8Nz=xbfg@mail.gmail.com>
On Mon, Dec 8, 2014, at 18:20, Dave Taht wrote:
> On Mon, Dec 8, 2014 at 8:56 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
> >> For netlink, we shouldn't be using arch_fast_hash() as a hashing
> >> discipline, but rather jhash() instead.
>
> I am not particularly happy with the amount of entropy in
>
> static inline u32 ipv6_addr_hash(const struct in6_addr *a)
> {
> #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG ==
> 64
> const unsigned long *ul = (const unsigned long *)a;
> unsigned long x = ul[0] ^ ul[1];
>
> return (u32)(x ^ (x >> 32));
> #else
> return (__force u32)(a->s6_addr32[0] ^ a->s6_addr32[1] ^
> a->s6_addr32[2] ^ a->s6_addr32[3]);
> #endif
> }
>
> is this worth improving somehow?
>
That's e.g. the reason why we have
commit 5a3da1fe9561828d0ca7eca664b16ec2b9bf0055
Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri Mar 15 11:32:30 2013 +0000
inet: limit length of fragment queue hash table bucket lists
Note, __ipv6_addr_jhash (xoring the upper 32 bit before jhashing them)
has the same problem. I currently cannot spot any problematic users in
the kernel, flow dissector hashes are insecure by nature, local
addresses normally don't have problems with hash collisions. But maybe I
should redo an audit. :)
Bye,
Hannes
^ permalink raw reply
* Re: [ovs-discuss] kernel panic receiving flooded VXLAN traffic with OVS
From: Jesse Gross @ 2014-12-08 17:33 UTC (permalink / raw)
To: Nicholas Bastin; +Cc: Jay Vosburgh, netdev, discuss@openvswitch.org
In-Reply-To: <CADmMkWdGhw3Rn3gMEtAV8ESZB-k+uAK=Ed=LwsShKOtyHdJS_g@mail.gmail.com>
On Sat, Dec 6, 2014 at 2:47 PM, Nicholas Bastin <nick.bastin@gmail.com> wrote:
> On Fri, Dec 5, 2014 at 4:51 PM, Jesse Gross <jesse@nicira.com> wrote:
>>
>> I don't think there is anything inherently wrong with aggregating TCP
>> segments in VXLAN that are not destined for the local host. This is
>> conceptually the same as doing aggregation for TCP packets where we
>> only perform L2 bridging - in theory we shouldn't look at the upper
>> layers but it is fine as long as we faithfully reconstruct it on the
>> way out.
>
>
> But you don't faithfully reconstruct what the user originally sent - in-path
> reassembly is always wrong, which is why hardware switches don't do it (by
> default, anyhow). If you configure a middlebox to do some kind of
> assembly/translation/whatever work for you, that's fine, but something that
> advertises itself as a "switch" or "router" should definitely not do this by
> default.
>
> If you reassemble frames you completely obviate any kind of PMTU-D or
> configured MTU that your user is using, and this breaks a lot of paths. We
> completely disable all GRO/TSO/etc., but if you are able to determine that a
> packet is not destined for the local host you should definitely not mutate
> it.
If you look at the implementation of GRO/TSO, I think you will see
that it does in fact faithfully reconstruct the original message and
path MTU discovery is preserved. On Linux systems, GRO is enabled by
default for all workloads - including those that do not result in
local termination such as bridging.
^ permalink raw reply
* Re: [PATCH net-next 2/3] netlink: IFLA_PHYS_SWITCH_ID to IFLA_PHYS_PARENT_ID
From: Andy Gospodarek @ 2014-12-08 17:49 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, sfeldma
In-Reply-To: <20141208164143.GI1885@nanopsycho.brq.redhat.com>
On Mon, Dec 08, 2014 at 05:41:43PM +0100, Jiri Pirko wrote:
> Mon, Dec 08, 2014 at 04:37:47PM CET, gospo@cumulusnetworks.com wrote:
> >On Mon, Dec 08, 2014 at 04:17:14PM +0100, Jiri Pirko wrote:
> >> Fri, Dec 05, 2014 at 07:02:16PM CET, gospo@cumulusnetworks.com wrote:
> >> >There has been much discussion about proper nomenclature to use for this
> >> >and I would prefer parent rather than calling every forwarding element a
> >> >switch.
> >>
> >> Andy, I must say I really do not like just plain "parent". It is really
> >> not clear what it means as it can mean 1000 things.
> >>
> >> I know "switch" is not ideal but everytime anyone is talking about these
> >> kind of forwarding devices, they use word "switch" even if it is not
> >> accurate and everyone knows what they are talking about. Nobody uses
> >> "parent".
> >
> >Well of course they are not going to use it until it's committed. ;-)
>
>
> Do you seriously expect people talking about "parents" instead of
"Parent device" -- absolutely
> "switches". I doubt that...
Agree to disagree, I guess!
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 17:58 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141208164650.GB29028@node.dhcp.inet.fi>
On Mon, Dec 08, 2014 at 06:46:50PM +0200, Kirill A. Shutemov wrote:
> I guess this crash is related to the patchset.
Might be. Do you have a reproducer for it?
It looks like the second VIRTUAL_BUG_ON() in __phys_addr(), most likely
from __pa(), from virt_to_page(), from
unsigned long addr = (unsigned long)v.iov_base, end;
size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1));
if (len > maxpages * PAGE_SIZE)
len = maxpages * PAGE_SIZE;
addr &= ~(PAGE_SIZE - 1);
for (end = addr + len; addr < end; addr += PAGE_SIZE)
get_page(*pages++ = virt_to_page(addr));
return len - *start;
in iov_iter_get_pages(). And that's ITER_KVEC case there... Further
call chain looks like dio_refill_pages(), from dio_get_page(), from
do_direct_io(), eventually from kernel_read() and finit_module(),
Presumably called on O_DIRECT descriptor...
I'll try to reproduce it here, but if you have any reliable reproducer, it
would be very welcome (and would make a useful addition to LTP and/or
xfstests).
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:07 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208164650.GB29028@node.dhcp.inet.fi>
On Mon, Dec 8, 2014 at 8:46 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> I guess this crash is related to the patchset.
Sounds likely.
> [ 102.338270] kernel BUG at /home/kas/git/public/linux-next/arch/x86/mm/physaddr.c:26!
So that's
VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
and the code disassembles to:
0: 48 01 f8 add %rdi,%rax
3: 48 39 c2 cmp %rax,%rdx
6: 72 1b jb 0x23
8: 0f b6 0d 9d 7a ec 00 movzbl 0xec7a9d(%rip),%ecx # 0xec7aac
f: 48 89 c2 mov %rax,%rdx
12: 48 d3 ea shr %cl,%rdx
15: 48 85 d2 test %rdx,%rdx
18: 75 09 jne 0x23
1a: 5d pop %rbp
1b: c3 retq
1c: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
23:* 0f 0b ud2 <-- trapping instruction
with thre relevant registers being
> [ 102.340011] RAX: 00004100174b4000 RBX: ffff880049c73b08 RCX: 0000000000000028
> [ 102.340011] RDX: 0000000000000041 RSI: ffff88015dc980a8 RDI: ffffc900174b4000
so we've taken the second case (the %rcx value is
"boot_cpu_data.x86_phys_bits", which is that "movzbl", and the %rdx
value is the shifted value of %rax).
So %rax seems to contain 'x' at that point, which means that 'y' should be
x - (__START_KERNEL_map - PAGE_OFFSET)
which means that the _original_ address should be that plus
__START_KERNEL_map, ie just x + PAGE_OFFSET.
So it smells like the original virtual address was that
ffffc900174b4000 that we still find in %rdi.
Which is in the vmalloc address space. So somebody used a vmalloc'ed
address and tried to convert it to a physical address in order to look
up the page.
Which is not a valid operation, and the BUG_ON() is definitely proper.
Now *why* something tried to do a virt_to_page() on a vmalloc'ed
address, that I leave to others.
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:08 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141208175805.GB22149@ZenIV.linux.org.uk>
On Mon, Dec 08, 2014 at 05:58:05PM +0000, Al Viro wrote:
> It looks like the second VIRTUAL_BUG_ON() in __phys_addr(), most likely
> from __pa(), from virt_to_page(), from
> unsigned long addr = (unsigned long)v.iov_base, end;
> size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1));
>
> if (len > maxpages * PAGE_SIZE)
> len = maxpages * PAGE_SIZE;
> addr &= ~(PAGE_SIZE - 1);
> for (end = addr + len; addr < end; addr += PAGE_SIZE)
> get_page(*pages++ = virt_to_page(addr));
> return len - *start;
> in iov_iter_get_pages(). And that's ITER_KVEC case there... Further
> call chain looks like dio_refill_pages(), from dio_get_page(), from
> do_direct_io(), eventually from kernel_read() and finit_module(),
> Presumably called on O_DIRECT descriptor...
FWIW, virt_to_page() is probably not OK to call on an address in the
middle of vmalloc'ed area, is it? Would
for (end = addr + len; addr < end; addr += PAGE_SIZE) {
if (is_vmalloc_addr(addr))
ACCESS_ONCE(*(char *)addr);
get_page(*pages++ = virt_to_page(addr));
}
be a safe replacement for the loop in the above?
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFz6izpVGxkziynRXg7zbHzteQyPJH1Kg91ZO3fzqn3LaQ@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:07:55AM -0800, Linus Torvalds wrote:
> Which is in the vmalloc address space. So somebody used a vmalloc'ed
> address and tried to convert it to a physical address in order to look
> up the page.
>
> Which is not a valid operation, and the BUG_ON() is definitely proper.
>
> Now *why* something tried to do a virt_to_page() on a vmalloc'ed
> address, that I leave to others.
iov_iter_get_pages() in ITER_KVEC case, trying to avoid get_user_pages_fast()
and getting it wrong. FWIW, the reproducer is finit_module(fd, ....)
where fd has been opened with O_DIRECT. In that case we get kernel_read()
on O_DIRECT and the buffer has just been vmalloc'ed.
What's the sane way to grab struct page * for a vmalloc'ed address?
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:14 UTC (permalink / raw)
To: Al Viro
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208180824.GC22149@ZenIV.linux.org.uk>
On Mon, Dec 8, 2014 at 10:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, virt_to_page() is probably not OK to call on an address in the
> middle of vmalloc'ed area, is it?
See my email that crossed yours. No it is not.
> Would
> for (end = addr + len; addr < end; addr += PAGE_SIZE) {
> if (is_vmalloc_addr(addr))
> ACCESS_ONCE(*(char *)addr);
> get_page(*pages++ = virt_to_page(addr));
> }
> be a safe replacement for the loop in the above?
No. That "ACCESS_ONCE()" does nothing. It reads a byte from 'addr' in
the vmalloc space, and might cause a page fault to make sure it's
mapped, but that is still a no-op.
You can't do "virt_to_page()" on anything but the normal 1:1 kernel
mappings (and only for non-highmem pages at that).
For a vmalloc() address, you'd have to actually walk the page tables.
Which is a f*cking horrible idea. Don't do it. We do have a
"vmalloc_to_page()" that does it, but the basic issue is that you damn
well shouldn't do IO on vmalloc'ed addresses. vmalloc'ed addresses
only exist in the first place to give a linear *virtual* mapping, if
you want physical pages you shouldn't have mixed it up with vmalloc in
the first place!
Where the hell does this crop up, and who does this insane thing
anyway? It's wrong. How did it ever work before?
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFxwrH4vsyw2ix=HqDK9Z=fw98xtOL8=57prm8qgVmAFfA@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:14:13AM -0800, Linus Torvalds wrote:
> For a vmalloc() address, you'd have to actually walk the page tables.
> Which is a f*cking horrible idea. Don't do it. We do have a
> "vmalloc_to_page()" that does it, but the basic issue is that you damn
> well shouldn't do IO on vmalloc'ed addresses. vmalloc'ed addresses
> only exist in the first place to give a linear *virtual* mapping, if
> you want physical pages you shouldn't have mixed it up with vmalloc in
> the first place!
>
> Where the hell does this crop up, and who does this insane thing
> anyway? It's wrong. How did it ever work before?
finit_module() with O_DIRECT descriptor. And I suspect that "not well"
is the answer - it used to call get_user_pages_fast() in that case.
I certainly had missed that insanity during the analysis - we don't do
a lot of O_DIRECT IO to/from kernel addresses of any sort... This
codepath allows it ;-/ Ability to trigger it is equivalent to ability
to run any code in kernel mode, so it's not an additional security hole,
but...
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:23 UTC (permalink / raw)
To: Al Viro
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208181401.GD22149@ZenIV.linux.org.uk>
On Mon, Dec 8, 2014 at 10:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> iov_iter_get_pages() in ITER_KVEC case, trying to avoid get_user_pages_fast()
> and getting it wrong. FWIW, the reproducer is finit_module(fd, ....)
> where fd has been opened with O_DIRECT. In that case we get kernel_read()
> on O_DIRECT and the buffer has just been vmalloc'ed.
Ugh. That's horrid. Do we need to even support O_DIRECT in that case?
In general, we should *not* do IO on vmalloc'ed areas, although at
least the non-O_DIRECT case where we just memcpy() it as if it came
from user space is much better.
Did this actually use to work? Or is it an issue of "the new iov_iter
is so generic that something that used to just return an error now
'works' and triggers the problem"?
> What's the sane way to grab struct page * for a vmalloc'ed address?
So "vmalloc_to_page()" should work.
However, it's actually fundamentally racy unless you can guarantee
that the vmalloc()'ed area in question is stable (so you had better
have done that allocation yourself, and be in control of the freeing,
rather than "we look up random vmalloc'ed addresses).
In general, it's really a horrible thing to use, and tends to be a big
red sign that "somebody misdesigned this badly"
Linus
^ permalink raw reply
* Re: [ovs-dev] OVS Kernel Datapath development
From: Pravin Shelar @ 2014-12-08 18:30 UTC (permalink / raw)
To: Thomas Graf; +Cc: netdev, dev@openvswitch.org
In-Reply-To: <20141208171506.GC2835@pox.localdomain>
On Mon, Dec 8, 2014 at 9:15 AM, Thomas Graf <tgraf@noironetworks.com> wrote:
> On 12/07/14 at 08:47pm, Pravin Shelar wrote:
>> Since the beginning OVS kernel datapath development is primarily done
>> on external OVS repo. Now we have mostly synced upstream and external
>> OVS. So we have decided to change this process. New process is as
>> follows.
>>
>> 1. OVS feature development that involves kernel datapath should be
>> done on net-next tree datapath.
>> 2. Such feature patch series should be posted on netdev and ovs-dev
>> mailing list.
>> 3. Once review is done for entire series, kernel and OVS userspace
>> patches will be merged in respective repo.
>> 4. After the merge developer is suppose to send patches for external
>> kernel datapath along with old kernel compatibility code. So that we
>> can keep external datapath insync.
>
> +1
>
> Just to be clear, by respective repo do you mean net-next/net or will
> you maintain a net-next branch on git.kernel.org and continue doing
> pull requests?
OVS patches will directly go to net-next/net tree. I am not planning
on maintaining any tree on git.kernel.org.
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:35 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFzKGsYKh1HALoeFGYQ46uMHww0WdLf5N-9duNTgMSSFxQ@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:23:26AM -0800, Linus Torvalds wrote:
> Did this actually use to work? Or is it an issue of "the new iov_iter
> is so generic that something that used to just return an error now
> 'works' and triggers the problem"?
Looks like it failed with EINVAL. Which might very well be the sane
reaction - if we run into a vmalloc/module address, act as if we failed
to get that page and exit the loop.
> > What's the sane way to grab struct page * for a vmalloc'ed address?
>
> So "vmalloc_to_page()" should work.
>
> However, it's actually fundamentally racy unless you can guarantee
> that the vmalloc()'ed area in question is stable (so you had better
> have done that allocation yourself, and be in control of the freeing,
> rather than "we look up random vmalloc'ed addresses).
If vfree(buffer) races with kernel_read() into buffer, we are so badly
fucked that stability of pointers to pages is the least of our concerns...
> In general, it's really a horrible thing to use, and tends to be a big
> red sign that "somebody misdesigned this badly"
More like "nobody has thought of that case", at a guess, but then I hadn't
been involved in finit_module() design - I don't even remember the discussions
around it. That would be what, something circa 3.7?
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:37 UTC (permalink / raw)
To: Al Viro
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208182012.GE22149@ZenIV.linux.org.uk>
On Mon, Dec 8, 2014 at 10:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> I certainly had missed that insanity during the analysis - we don't do
> a lot of O_DIRECT IO to/from kernel addresses of any sort... This
> codepath allows it ;-/ Ability to trigger it is equivalent to ability
> to run any code in kernel mode, so it's not an additional security hole,
> but...
Is there any chance we could just return EINVAL for this case?
Who does O_DIRECT on module load anyway? If this is only for
finit_module(), that uses "kernel_read()", and maybe we could just
make sure that the kernel_read() function never ever uses the
direct-IO paths?
[ Time passes, I look at the code ]
Oh crap. So the reason it triggers seems to be that we basically get a
random file descriptor that we didn't open, and then we have
vfs_read() ->
xfs_file_operations->read() ->
ew_sync_read() ->
xfs_file_operations->read_iter()
xfs_file_read_iter()
and we are stuck with this iterator that really just wants to do copies.
How about we make "kernel_read()" just clear O_DIRECT? Does that fix
it to just use copies?
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFxrrTV-xBbvEzVKsXRVKyvZhQZj-6BNDpTsG4J5k6CJEg@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:37:51AM -0800, Linus Torvalds wrote:
> How about we make "kernel_read()" just clear O_DIRECT? Does that fix
> it to just use copies?
Umm... clearing O_DIRECT on struct file that might have other references
to it isn't nice, to put it mildly...
Frankly, stopping iov_iter_get_pages() on the first page in vmalloc/module
space looks like the sanest strategy anyway. We'd get the same behaviour
as we used to, and as for finit_module(2), well... put "fails if given
an O_DIRECT descriptor" and be done with that.
Alternatively, we can really go for
page = is_vmalloc_or_module_addr(addr) ? vmalloc_to_page(addr)
: virt_to_page(addr)
*pages++ = get_page(page);
and have the fucker work. Stability is up to the caller, of course -
reading into buffer that might get freed (and reused) under you has much
worse problems...
^ permalink raw reply
* RE: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set
From: Arad, Ronen @ 2014-12-08 18:40 UTC (permalink / raw)
To: Roopa Prabhu, Scott Feldman, Netdev
Cc: Jirí Pírko, Jamal Hadi Salim, Benjamin LaHaise,
Thomas Graf, john fastabend, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, David S. Miller, shm@cumulusnetworks.com,
Andy Gospodarek
In-Reply-To: <5484B773.7000809@cumulusnetworks.com>
> -----Original Message-----
> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
> Sent: Sunday, December 07, 2014 12:24 PM
> To: Scott Feldman
> Cc: Arad, Ronen; Netdev; Jirí Pírko; Jamal Hadi Salim; Benjamin LaHaise;
> Thomas Graf; john fastabend; stephen@networkplumber.org; John Linville;
> nhorman@tuxdriver.com; Nicolas Dichtel; vyasevic@redhat.com; Florian
> Fainelli; buytenh@wantstofly.org; Aviad Raveh; David S. Miller;
> shm@cumulusnetworks.com; Andy Gospodarek
> Subject: Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic
> if feature flag set
>
> On 12/5/14, 10:54 PM, Scott Feldman wrote:
> > On Fri, Dec 5, 2014 at 3:21 PM, Arad, Ronen <ronen.arad@intel.com>
> wrote:
> >>
> >>> -----Original Message-----
> >>> From: netdev-owner@vger.kernel.org [mailto:netdev-
> >>> owner@vger.kernel.org] On Behalf Of Roopa Prabhu
> >>> Sent: Thursday, December 04, 2014 11:02 PM
> >>> To: Scott Feldman
> >>> Cc: Jiří Pírko; Jamal Hadi Salim; Benjamin LaHaise; Thomas Graf;
> >>> john fastabend; stephen@networkplumber.org; John Linville;
> >>> nhorman@tuxdriver.com; Nicolas Dichtel; vyasevic@redhat.com; Florian
> >>> Fainelli; buytenh@wantstofly.org; Aviad Raveh; Netdev; David S.
> >>> Miller; shm@cumulusnetworks.com; Andy Gospodarek
> >>> Subject: Re: [PATCH 2/3] bridge: offload bridge port attributes to
> >>> switch asic if feature flag set
> >>>
> >>> On 12/4/14, 10:41 PM, Scott Feldman wrote:
> >>>> On Thu, Dec 4, 2014 at 6:26 PM, <roopa@cumulusnetworks.com>
> wrote:
> >>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> >>>>>
> >>>>> This allows offloading to switch asic without having the user to
> >>>>> set any flag. And this is done in the bridge driver to rollback
> >>>>> kernel settings on hw offload failure if required in the future.
> >>>>>
> >>>>> With this, it also makes sure a notification goes out only after
> >>>>> the attributes are set both in the kernel and hw.
> >>>> I like this approach as it streamlines the steps for the user in
> >>>> setting port flags. There is one case for FLOODING where you'll
> >>>> have to turn off flooding for both, and then turn on flooding in
> >>>> hw. You don't want flooding turned on on kernel and hw.
> >>> ok, maybe using the higher bits as in
> >>> https://patchwork.ozlabs.org/patch/413211/
> >>>
> >>> might help with that. Let me think some more.
> >>>>> ---
> >>>>> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++-
> >>>>> 1 file changed, 26 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> >>>>> index
> >>>>> 9f5eb55..ce173f0 100644
> >>>>> --- a/net/bridge/br_netlink.c
> >>>>> +++ b/net/bridge/br_netlink.c
> >>>>> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct
> >>> nlmsghdr *nlh)
> >>>>> afspec, RTM_SETLINK);
> >>>>> }
> >>>>>
> >>>>> + if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
> >>>>> + dev->netdev_ops->ndo_bridge_setlink) {
> >>>>> + int ret = dev->netdev_ops->ndo_bridge_setlink(dev,
> >>>>> + nlh);
> >>>> I think you want to up-level this to net/core/rtnetlink.c because
> >>>> you're only enabling the feature for one instance of a driver that
> >>>> implements ndo_bridge_setlink: the bridge driver. If another
> >>>> driver was MASTER and implemented ndo_bridge_setlink, you'd want
> >>>> same check to push setting down to SELF port driver.
> >>> yeah, i thought about that. But i moved it here so that rollback
> >>> would be easier.
> >> There is a need for propagating setlink/dellink requests down multiple
> levels.
> >> The use-case I have in mind is a bridge at the top, team/bond in the
> middle, and port devices at the bottom.
> >> A setlink for VLAN filtering attributes would come with MASTER flag set,
> and either port or bond/team netdev.
> >> How would this be handled?
> >>
> >> The propagation rules between bridge and enslaved port device could be
> different from those between bond/team and enslaved devices.
> >> The current bridge driver does not propagate VLAN filtering from bridge to
> its ports as each port could have different configuration. In a case of a
> bond/team all members need to have the same configuration such that the a
> bond/team would be indistinguishable from a simple port.
> >>
> >> Therefore rtnetlink.c might not have the knowledge for propagation
> across multiple levels.
> >> It seems that each device which implements
> ndo_bridge_setlink/ndo_bridge_dellink and could have master role, need to
> take care of propagation to its slaves.
> > Thanks Ronen for bringing up this use-case of stacked masters. I
> > think for VLAN filtering, the stacked master case is handled, not by
> > ndo_setlink/dellink at each level, but with ndo_vlan_rx_kill_vid and
> > ndo_vlan_rx_add_vid. So the switch port driver can know VLAN
> > membership for port even if port is under bond which is under bridge,
> > by using ndo_vlan_rx_xxx and setting NETIF_F_HW_VLAN_CTAG_FILTER.
> The
> > bonding driver's ndo_vlan_rx_xxx handlers seem to keep ports in bond
> > VLAN membership consistent across bond.
This could have worked if the entire VLAN filtering policy was propagated. Currently only VLAN membership is propagated using ndo_vlan_rx_add_vid.
Missing are untagged flag for egress and PVID flag.
> >
> > But in general, ndo_setlink/dellink don't work for the stack use-case
> > for other non-VLAN attributes. Maybe the answer is to use the VLAN
> > propogation model for other attributes. ndo_setlink/dellink/getlink
> > have enough weird-isms it might be time to define cleaner ndo ops to
> > propagate the other attrs down.
> And, only the switch asic driver is interested in these attrs. So, seems like for
> these cases, we need to send these attrs to the switchdriver directly instead
> of going through the stack of netdevs ?. see my response to ronen's other
> email.
>
> Thanks,
> Roopa
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Marcel Holtmann @ 2014-12-08 18:50 UTC (permalink / raw)
To: Pali Rohár
Cc: Greg Kroah-Hartman, Ming Lei, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <201412081811.46943@pali>
Hi Pali,
>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>>>>>> /**
>>>>>>
>>>>>> + * request_firmware_prefer_user: - prefer usermode
>>>>>> helper for loading firmware + * @firmware_p: pointer to
>>>>>> firmware image
>>>>>> + * @name: name of firmware file
>>>>>> + * @device: device for which firmware is being loaded
>>>>>> + *
>>>>>> + * This function works pretty much like
>>>>>> request_firmware(), but it prefer + * usermode helper. If
>>>>>> usermode helper fails then it fallback to direct access.
>>>>>> + * Usefull for dynamic or model specific firmware data.
>>>>>> + **/
>>>>>> +int request_firmware_prefer_user(const struct firmware
>>>>>> **firmware_p, + const char
>>>>>> *name, struct device *device) +{
>>>>>> + int ret;
>>>>>> + __module_get(THIS_MODULE);
>>>>>> + ret = _request_firmware(firmware_p, name, device,
>>>>>> + FW_OPT_UEVENT |
>>>>>> FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>>>
>>>>> I'd like to introduce request_firmware_user() which only
>>>>> requests firmware from user space, and this way is simpler
>>>>> and more flexible since we have request_firmware_direct()
>>>>> already.
>>>>
>>>> Why would a driver care about what program provides the
>>>> firmware? It shouldn't at all, and we want to get rid of
>>>> the userspace firmware loader, not encourage drivers to
>>>> use it "exclusively" at all.
>>>
>>> Do not remove it! Without userspace firmware loader it is
>>> impossible to load dynamic firmware files.
>>
>> why is this dynamic in the first place. It does not sound like
>> dynamic data to me at all. This is like the WiFi MAC
>> address(es) or Bluetooth BD_ADDR. They are all static
>> information. The only difference is that they are on the host
>> accessibly filesystem or storage and not on the device
>> itself.
>>
>> To be honest, for Bluetooth we solved this now. If the device
>> is missing key information like the calibration data or
>> BD_ADDR, then it comes up unconfigured. A userspace process
>> can then go and load the right data into it and then the
>> device becomes available as Bluetooth device.
>>
>> Trying to use request_firmware to load some random data and
>> insist on going through userspace helper for that sounds
>> crazy to me. Especially since we are trying hard to get away
>> from the userspace loader. Forcing to keep it for new stuff
>> sounds backwards to me.
>>
>> With the special Nokia partition in mind, why hasn't this been
>> turned into a mountable filesystem or into a driver/subsystem
>> that can access the data direct from the kernel. I advocated
>> for this some time ago. Maybe there should be a special
>> subsystem for access to these factory persistent information
>> that drivers then just can access. I seem to remember that
>> some systems provide these via ACPI. Why does the ARM
>> platform has to be special here?
>>
>> And the problem of getting Ethernet and WiFi MAC address and
>> Bluetooth BD_ADDR comes up many many times. Why not have
>> something generic here. And don't tell me request_firmware is
>> that generic solution ;)
>>
>> Regards
>>
>> Marcel
>
> Hi Marcel. I think you did not understand this problem. This
> discussion is not about mac address. Please read email thread
> again and if there are some unclear pars, then ask. Thanks!
I think that I pretty clearly understand the problem. Calibration data, MAC address, what is the difference? For me this is all the same. It is data that is specific to a device or type of devices and it is stored somewhere else. In most cases in some immutable memory/flash area.
What you want is access to this data since the kernel driver needs it. Do I get this so far ;)
So my take is that request_firmware is not the right way to get this data. Or more precisely make sure that this data is available to kernel drivers. And what I am seeing here is that instead of actually solving the bigger problem, we just hack around it with request_firmware. Now surprisingly the request_firmware loads files directly from the kernel and all the hacks do not work anymore.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH RFC] pci: Control whether VFs are probed on pci_enable_sriov
From: Eli Cohen @ 2014-12-08 18:52 UTC (permalink / raw)
To: Don Dutile
Cc: Yuval Mintz, Eli Cohen, bhelgaas@google.com, David Miller,
linux-pci, netdev, ogerlitz@mellanox.com, yevgenyp@mellanox.com
In-Reply-To: <5484C77D.7060501@redhat.com>
On Sun, Dec 07, 2014 at 04:32:45PM -0500, Don Dutile wrote:
> >
> >Does a one-shot flag suffice? E.g., consider assigning a VF to VM and
> >than shutting down the VM. Assuming this feature is disabled,
> >the VF didn't appear on the hypervisor prior to the assignment but
> >will appear after its shutdown.
> >
> +1 to this question.
> All I see is a one-shot savings in VF configuration time at pci_sriov_enable() time.
> Please explain why this is so important for mlx5 (sriov) operation?
>
This is not a specific mlx5 requirement. The rational is that you
don't want to probe the virtual fucntions at the hypervisor just
because they appear there all of the sudden. You are likely to want to
assign some of them to VMs so why probe them and then remove them? It
makes sense to me that other drivers would also like this kind of
fucntionality.
> Can the vf driver probe be called & exit early the first time, and perform full
> (host) configuration thereafter?
>
Yes, this can be done but I was thinking that other drivers could also
benefit from such functionality.
^ 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