Re: [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. Please break it up into one-patch-per-subsystem, like normal, and get it merged that way. Sending us a patch, without even a diffstat to review, isn't going to get you very far... thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drivers/virt: vmgenid: add vm generation id driver
On Sat, Oct 17, 2020 at 8:09 PM Alexander Graf wrote: > There are applications way beyond that though. What do you do with > applications that already consumed randomness? For example a cached pool > of SSL keys. Or a higher level language primitive that consumes > randomness and caches its seed somewhere in an internal data structure. For deterministic protection, those would also have to poll some memory location that tells them whether the VmGenID changed: 1. between reading entropy from their RNG pool and using it 2. between collecting data from external sources (user input, clock, ...) and encrypting it and synchronously shoot down the connection if a change happened. If e.g. an application inside the VM has an AES-GCM-encrypted TLS connection and, directly after the VM is restored, triggers an application-level timeout that sends some fixed message across the connection, then the TLS library must guarantee that either the VM was already committed to sending exactly that message before the VM was forked or the message will be blocked. If we don't do that, an attacker who captures both a single packet from the forked VM and traffic from the old VM can decrypt the next message from the old VM after the fork (because AES-GCM is like AES-CTR plus an authenticator, and CTR means that when keystream reuse occurs and one of the plaintexts is known, the attacker can simply recover the other plaintext using XOR). (Or maybe, in disaster failover environments, TLS 1.3 servers could get away with rekeying the connection instead of shooting it down? Ask your resident friendly cryptographer whether that would be secure, I am not one.) I don't think a mechanism based around asynchronously telling the application and waiting for it to confirm the rotation at a later point is going to cut it; we should have some hard semantics on when an application needs to poll this value. > Or even worse: your system's host ssh key. Mmmh... I think I normally would not want a VM to reset its host ssh key after merely restoring a snapshot though? And more importantly, Microsoft's docs say that they also change the VmGenID on disaster failover. I think you very much wouldn't want your server to lose its host key every time disaster failover happens. On the other hand, after importing a public VM image, it might be a good idea. I guess you could push that responsibility on the user, by adding an option to the sshd_config that tells OpenSSH whether the host key should be rotated on an ID change or not... but that still would not be particularly pretty. Ideally we would have the host tell us what type of events happened to the VM, or something like that... or maybe even get the host VM management software to ask the user whether they're importing a public image... I really feel like with Microsoft's current protocol, we don't get enough information to figure out what we should do about private long-term authentication keys. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 9:10 AM wrote:
>
> From: Tom Rix
>
> This is a upcoming change to clean up a new warning treewide.
> I am wondering if the change could be one mega patch (see below) or
> normal patch per file about 100 patches or somewhere half way by collecting
> early acks.
>
> clang has a number of useful, new warnings see
> https://clang.llvm.org/docs/DiagnosticsReference.html
>
> This change cleans up -Wunreachable-code-break
> https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break
> for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64.
>
> The method of fixing was to look for warnings where the preceding statement
> was a simple statement and by inspection made the subsequent break unneeded.
> In order of frequency these look like
>
> return and break
>
> switch (c->x86_vendor) {
> case X86_VENDOR_INTEL:
> intel_p5_mcheck_init(c);
> return 1;
> - break;
>
> goto and break
>
> default:
> operation = 0; /* make gcc happy */
> goto fail_response;
> - break;
>
> break and break
> case COLOR_SPACE_SRGB:
> /* by pass */
> REG_SET(OUTPUT_CSC_CONTROL, 0,
> OUTPUT_CSC_GRPH_MODE, 0);
> break;
> - break;
>
> The exception to the simple statement, is a switch case with a block
> and the end of block is a return
>
> struct obj_buffer *buff = r->ptr;
> return scnprintf(str, PRIV_STR_SIZE,
> "size=%u\naddr=0x%X\n", buff->size,
> buff->addr);
> }
> - break;
>
> Not considered obvious and excluded, breaks after
> multi level switches
> complicated if-else if-else blocks
> panic() or similar calls
>
> And there is an odd addition of a 'fallthrough' in drivers/tty/nozomi.c
[..]
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index 5a7c80053c62..2f250874b1a4 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -200,11 +200,10 @@ ssize_t nd_namespace_store(struct device *dev,
> }
> break;
> default:
> len = -EBUSY;
> goto out_attach;
> - break;
> }
Acked-by: Dan Williams
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. > > clang has a number of useful, new warnings see > https://clang.llvm.org/docs/DiagnosticsReference.html > > This change cleans up -Wunreachable-code-break > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. Early acks/individual patches by subsystem would be good. Better still would be an automated cocci script. The existing checkpatch test for UNNECESSARY_BREAK has a few too many false positives. >From a script run on next on July 28th: arch/arm/mach-s3c24xx/mach-rx1950.c:266: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:38: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:41: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:684: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:687: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:690: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:693: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:697: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:700: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:276: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:279: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:282: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:287: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:290: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:76: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:79: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:231: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:234: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:237: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:240: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/net/bpf_jit_comp.c:455: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2047: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2077: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/sh/boards/mach-landisk/gio.c:111: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1734: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1738: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/microcode/amd.c:218: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:107: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:132: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:147: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:158: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/ata/libata-scsi.c:3973: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/base/power/main.c:366: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/block/xen-blkback/blkback.c:1272: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/ipmi/ipmi_devintf.c:493: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/lp.c:625: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/mwave/mwavedd.c:406: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/cpufreq/
Re: [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 20:21 +0200, Julia Lawall wrote: > On Sat, 17 Oct 2020, Joe Perches wrote: > > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > > > clang has a number of useful, new warnings see > > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > > > This change cleans up -Wunreachable-code-break > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > > > Early acks/individual patches by subsystem would be good. > > Better still would be an automated cocci script. > > Coccinelle is not especially good at this, because it is based on control > flow, and a return or goto diverts the control flow away from the break. > A hack to solve the problem is to put an if around the return or goto, but > that gives the break a meaningless file name and line number. I collected > the following list, but it only has 439 results, so fewer than clang. But > maybe there are some files that are not considered by clang in the x86 > allyesconfig configuration. > > Probably checkpatch is the best solution here, since it is not > configuration sensitive and doesn't care about control flow. Likely the clang compiler is the best option here. It might be useful to add -Wunreachable-code-break to W=1 or just always enable it if it isn't already enabled. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..3819787579d5 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -32,6 +32,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS += $(call cc-option, -Wunreachable-code-break) # The following turn off the warnings enabled by -Wextra KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-sign-compare (and thank you Tom for pushing this forward) checkpatch can't find instances like: case FOO: if (foo) return 1; else return 2; break; As it doesn't track flow and relies on the number of tabs to be the same for any goto/return and break; checkpatch will warn on: case FOO: ... goto bar; break; ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drivers/virt: vmgenid: add vm generation id driver
On Fri, Oct 16, 2020 at 6:40 PM Jann Horn wrote: > > [adding some more people who are interested in RNG stuff: Andy, Jason, > Theodore, Willy Tarreau, Eric Biggers. also linux-api@, because this > concerns some pretty fundamental API stuff related to RNG usage] > > On Fri, Oct 16, 2020 at 4:33 PM Catangiu, Adrian Costin > wrote: > > - Background > > > > The VM Generation ID is a feature defined by Microsoft (paper: > > http://go.microsoft.com/fwlink/?LinkId=260709) and supported by > > multiple hypervisor vendors. > > > > The feature is required in virtualized environments by apps that work > > with local copies/caches of world-unique data such as random values, > > uuids, monotonically increasing counters, etc. > > Such apps can be negatively affected by VM snapshotting when the VM > > is either cloned or returned to an earlier point in time. > > > > The VM Generation ID is a simple concept meant to alleviate the issue > > by providing a unique ID that changes each time the VM is restored > > from a snapshot. The hw provided UUID value can be used to > > differentiate between VMs or different generations of the same VM. > > > > - Problem > > > > The VM Generation ID is exposed through an ACPI device by multiple > > hypervisor vendors but neither the vendors or upstream Linux have no > > default driver for it leaving users to fend for themselves. > > > > Furthermore, simply finding out about a VM generation change is only > > the starting point of a process to renew internal states of possibly > > multiple applications across the system. This process could benefit > > from a driver that provides an interface through which orchestration > > can be easily done. > > > > - Solution > > > > This patch is a driver which exposes the Virtual Machine Generation ID > > via a char-dev FS interface that provides ID update sync and async > > notification, retrieval and confirmation mechanisms: > > > > When the device is 'open()'ed a copy of the current vm UUID is > > associated with the file handle. 'read()' operations block until the > > associated UUID is no longer up to date - until HW vm gen id changes - > > at which point the new UUID is provided/returned. Nonblocking 'read()' > > uses EWOULDBLOCK to signal that there is no _new_ UUID available. > > > > 'poll()' is implemented to allow polling for UUID updates. Such > > updates result in 'EPOLLIN' events. > > > > Subsequent read()s following a UUID update no longer block, but return > > the updated UUID. The application needs to acknowledge the UUID update > > by confirming it through a 'write()'. > > Only on writing back to the driver the right/latest UUID, will the > > driver mark this "watcher" as up to date and remove EPOLLIN status. > > > > 'mmap()' support allows mapping a single read-only shared page which > > will always contain the latest UUID value at offset 0. > > It would be nicer if that page just contained an incrementing counter, > instead of a UUID. It's not like the application cares *what* the UUID > changed to, just that it *did* change and all RNGs state now needs to > be reseeded from the kernel, right? And an application can't reliably > read the entire UUID from the memory mapping anyway, because the VM > might be forked in the middle. > > So I think your kernel driver should detect UUID changes and then turn > those into a monotonically incrementing counter. (Probably 64 bits > wide?) (That's probably also a little bit faster than comparing an > entire UUID.) > > An option might be to put that counter into the vDSO, instead of a > separate VMA; but I don't know how the other folks feel about that. > Andy, do you have opinions on this? That way, normal userspace code > that uses this infrastructure wouldn't have to mess around with a > special device at all. And it'd be usable in seccomp sandboxes and so > on without needing special plumbing. And libraries wouldn't have to > call open() and mess with file descriptor numbers. The vDSO might be annoyingly slow for this. Something like the rseq page might make sense. It could be a generic indication of "system went through some form of suspend". ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 05/29] virtio-mem: generalize check for added memory
> Am 17.10.2020 um 00:38 schrieb Wei Yang : > > On Fri, Oct 16, 2020 at 12:32:50PM +0200, David Hildenbrand wrote: > Ok, I seems to understand the logic now. > > But how we prevent ONLINE_PARTIAL memory block get offlined? There are > three > calls in virtio_mem_set_fake_offline(), while all of them adjust page's > flag. > How they hold reference to struct page? Sorry, I should have given you the right pointer. (similar to my other reply) We hold a reference either via 1. alloc_contig_range() >>> >>> I am not familiar with this one, need to spend some time to look into. >> >> Each individual page will have a pagecount of 1. >> >>> 2. memmap init code, when not calling generic_online_page(). >>> >>> I may miss some code here. Before online pages, memmaps are allocated in >>> section_activate(). They are supposed to be zero-ed. (I don't get the exact >>> code line.) I am not sure when we grab a refcount here. >> >> Best to refer to __init_single_page() -> init_page_count(). >> >> Each page that wasn't onlined via generic_online_page() has a refcount >> of 1 and looks like allocated. >> > > Thanks, I see the logic. > >online_pages() >move_pfn_range_to_zone() --- 1) >online_pages_range() --- 2) > > At 1), __init_single_page() would set page count to 1. At 2), > generic_online_page() would clear page count, while the call back would not. > > Then I am trying to search the place where un-zero page count prevent offline. > scan_movable_pages() would fail, since this is a PageOffline() and has 1 page > count. > > So the GUARD we prevent offline partial-onlined pages is > >(PageOffline && page_count) > > And your commit aa218795cb5fd583c94f > > mm: Allow to offline unmovable PageOffline() pages via MEM_GOING_OFFLINE > > is introduced to handle this case. > > That's pretty clear now. > I‘m happy to see that I am no longer the only person that understands all this magic :) Thanks for having a look / reviewing! >> -- >> Thanks, >> >> David / dhildenb > > -- > Wei Yang > Help you, Help me > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
