Re: [PATCH 1/2] PCI: ASPM exit link state code could skip devices
On Thu, 31 Jan 2013, Myron Stowe wrote: > On Fri, 2013-01-18 at 13:23 -0500, Joe Lawrence wrote: > > From 3a51bbadba6c6e144aa5176c8112eb449325 Mon Sep 17 00:00:00 2001 > > From: Joe Lawrence > > Date: Tue, 15 Jan 2013 14:51:57 -0500 > > Subject: [PATCH 1/2] PCI: ASPM exit link state code could skip devices > > > > On PCI bus hotplug removal, pcie_aspm_exit_link_state could potentially skip > > parent devices that have link_state allocated. Instead of exiting early if > > a given device is not PCIe, check that the device's parent is PCIe. This > > enables pcie_aspm_exit_link_state to properly clean up parent link_state > > when > > the last function in a slot might not be PCIe. > > > > Reviewed-by: David Bulkow > > Signed-off-by: Joe Lawrence > > --- > > drivers/pci/pcie/aspm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index b52630b..6122447 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -634,7 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > > struct pci_dev *parent = pdev->bus->self; > > struct pcie_link_state *link, *root, *parent_link; > > > > - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) > > + if (!parent || !pci_is_pcie(parent) || !parent->link_state) > > return; > > if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && > > (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) > > Joe: > > Bjorn and I looked at this again today and think the checks that occur > early in 'pcie_aspm_exit_link_state()' could be simplified (as shown > below) to avoid issues such as you are encountering. > > I think part of the confusion concerning the asymmetry between > 'pcie_aspm_init_link_state()' and 'pcie_aspm_exit_link_state()' is that > the former is passed in a pointer to a bridge device whereas the latter > is passed in a pointer to a end-point device. Frankly, I find the > entire driver to be confusing and very hard to understand. > > Could you please test with the following patch and let us know the > results? > > > PCI: ASPM exit link state code is skipping devices > > From: Myron Stowe > > On PCI bus hotplug removal, 'pcie_aspm_exit_link_state' can potentially > skip parent devices that have link_state allocated. Instead of exiting > early if a given device in not PCIe, check whether or not the device's > parent has link_state allocated. This enables 'pcie_aspm_exit_link_state' > to properly clean up parent link_state when the last function in a slot > might not be PCIe. > > Reported-by: Joe Lawrence > Signed-off-by: Myron Stowe > --- > > drivers/pci/pcie/aspm.c |5 + > 1 files changed, 1 insertions(+), 4 deletions(-) > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 8474b6a..aa52727 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -634,10 +634,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > struct pci_dev *parent = pdev->bus->self; > struct pcie_link_state *link, *root, *parent_link; > > - if (!pci_is_pcie(pdev) || !parent || !parent->link_state) > - return; > - if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) && > - (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) > + if (!parent || !parent->link_state) > return; > > down_read(&pci_bus_sem); > > > Hi Myron, The patch tests eliminates the reported crash when hotplugging our problem PCI bus. The additional checks removed from pcie_aspm_exit_link_state (ie, the simplification) do appear to be unnecessary as pcie_aspm_init_link_state is already preventing such devices from allocating link state. Thanks, -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Please add to stable: module: don't unlink the module until we've removed all exposure.
On Tue, 04 Jun 2013 15:26:28 +0930 Rusty Russell wrote: > Do you have a backtrace of the 3.9.4 crash? You can add "CFLAGS_module.o > = -O0" to get a clearer backtrace if you want... Hi Rusty, See my 3.9 stack traces below, which may or may not be what Ben had been seeing. If you like, I can try a similar loop as the one you were testing in the other email. Regards, -- Joe *** First instance *** [ cut here ] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0xd4/0x100() Hardware name: ftServer 6400 sysfs: cannot create duplicate filename '/module/mgag200' Modules linked in: enclosure(+) mgag200(+) ghash_clmulni_intel(+) pcspkr joydev osst vhost_net st tun macvtap macvlan uinput raid1 mpt2sas(OF) raid_class qla2xxx(OF) scsi_transport_fc scsi_transport_sas sd_mod(OF) usb_storage scsi_tgt scsi_hbas(OF) i2c_algo_bit drm_kms_helper ttm drm i2c_core Pid: 733, comm: systemd-udevd Tainted: GF O 3.9.0sra_new+ #1 Call Trace: [] warn_slowpath_common+0x7f/0xc0 [] warn_slowpath_fmt+0x46/0x50 [] ? strlcat+0x65/0x90 [] sysfs_add_one+0xd4/0x100 [] create_dir+0x78/0xd0 [] sysfs_create_dir+0x86/0xe0 [] kobject_add_internal+0xa8/0x270 [] kobject_init_and_add+0x63/0x90 [] load_module+0x12dd/0x2890 [] ? ddebug_proc_open+0xc0/0xc0 [] sys_init_module+0xea/0x140 [] system_call_fastpath+0x16/0x1b ---[ end trace 247a5f5f82ef192d ]--- [ cut here ] WARNING: at lib/kobject.c:196 kobject_add_internal+0x204/0x270() Hardware name: ftServer 6400 kobject_add_internal failed for mgag200 with -EEXIST, don't try to register things with the same name in the same directory. Modules linked in:0m] Started Conf mdio(+) coretemp(+) crc32c_intel(+) dca(+) enclosure(+) mgag200(+) ghash_clmulni_intel pcspkr joydev osst vhost_net st tun macvtap macvlan uinput raid1 mpt2sas(OF) raid_class qla2xxx(OF) scsi_transport_fc scsi_transport_sas sd_mod(OF) usb_storage scsi_tgt scsi_hbas(OF) i2c_algo_bit drm_kms_helper ttm drm i2c_core Pid: 733, comm: systemd-udevd Tainted: GF W O 3.9.0sra_new+ #1 Call Trace: [] warn_slowpath_common+0x7f/0xc0 [] warn_slowpath_fmt+0x46/0x50 [] kobject_add_internal+0x204/0x270 [] kobject_init_and_add+0x63/0x90 [] load_module+0x12dd/0x2890 [] ? ddebug_proc_open+0xc0/0xc0 [] sys_init_module+0xea/0x140 [] system_call_fastpath+0x16/0x1b ---[ end trace 247a5f5f82ef192e ]--- *** Second instance *** mgag200: module is already loaded igb: Intel(R) Gigabit Ethernet Network Driver - version 4.1.2-k BUG: unable to handle kernel paging request at a01d060c IP: [] kobject_del+0x16/0x40 PGD 1c0f067 PUD 1c10063 PMD 851372067 PTE 0 Oops: 0002 [#1] SMP Modules linked in: ixgbe(OF+) igb(OF+) mgag200(+) ptp pps_core mdio dca coretemp crc32c_intel pcspkr ghash_clmulni_intel vhost_net tun macvtap macvlan uinput raid1 usb_storage mpt2sas(OF) raid_class qla2xxx(OF) scsi_transport_fc scsi_transport_sas sd_mod(OF) scsi_tgt scsi_hbas(OF) i2c_algo_bit drm_kms_helper ttm drm i2c_core CPU 28 Pid: 719, comm: systemd-udevd Tainted: GF O 3.9.0sra_new+ #1 Stratus ftServer 6400/G7LAZ RIP: 0010:[] [] kobject_del+0x16/0x40 RSP: 0018:88103814fd08 EFLAGS: 00010292 RAX: 0200 RBX: a01d05d0 RCX: 000100250004 RDX: 88103814ffd8 RSI: 00250004 RDI: 0246 RBP: 88103814fd18 R08: 88103814fa80 R09: R10: 88085f821d40 R11: 0025 R12: 81c412c0 R13: 880852c8cfc0 R14: a01e0580 R15: a01e0598 FS: 7fc98fe6c840() GS:88107fd8() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: a01d060c CR3: 001038137000 CR4: 000407e0 DR0: DR1: DR2: DR3: DR6: 0ff0 DR7: 0400 Process systemd-udevd (pid: 719, threadinfo 88103814e000, task 8810380d98a0) Stack: 88103814fd18 a01d05d0 88103814fd48 81313302 88103814fd78 a01d05d0 a01d05d0 ffea 88103814fd68 8131348b 8000 88103814fee8 Call Trace: [] kobject_cleanup+0x62/0x1b0 [] kobject_put+0x2b/0x60 [] load_module+0x2881/0x2890 [] ? ddebug_proc_open+0xc0/0xc0 [] sys_init_module+0xea/0x140 [] system_call_fastpath+0x16/0x1b Code: 02 00 00 48 8b 5d f0 4c 8b 65 f8 c9 c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 85 ff 74 22 e8 7a fc f0 ff <80> 63 3c fd 48 89 df e8 6e ff ff ff 48 8b 7b 18 e8 d5 01 00 00 RIP [] kobject_del+0x16/0x40 RSP CR2: a01d060c ---[ end trace e320c2319820c81a ]--- Kernel panic - not syncing: Fatal exception -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Please add to stable: module: don't unlink the module until we've removed all exposure.
On Tue, 4 Jun 2013, Joe Lawrence wrote:
> Hi Rusty,
>
> See my 3.9 stack traces below, which may or may not be what Ben had
> been seeing. If you like, I can try a similar loop as the one you were
> testing in the other email.
With a modified version of your module load/unload loop (only needed
insmod as the module initialization routine returns -EINVAL to mimic
mgag200 with incorrect modeset value). This crashed right out of the
chute on 3.9.4 ... still running OK with 3.9 + commit 944a1fa "module:
don't unlink the module until we've removed all exposure".
-- Joe
test_mod.c :
#include
#include
MODULE_LICENSE("GPL");
static int test_mod_init(void) { return -EINVAL; }
static void test_mod_exit(void) {}
module_init(test_mod_init);
module_exit(test_mod_exit);
from the console log :
test_mod: module verification failed: signature and/or required key missing -
tainting kernel
[ cut here ]
WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0xd4/0x100()
Hardware name: ftServer 6400
sysfs: cannot create duplicate filename '/module/test_mod'
Modules linked in: test_mod(OF+) ebtable_nat nf_conntrack_netbios_ns
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat_ipv4 nf_nat iptable_mangle
nf_conntrack_ipv4 nf_defrag_ipv4 bonding xt_conntrack nf_conntrack ib_iser
rdma_cm ebtable_filter ib_addr ebtables iw_cm ib_cm ib_sa ib_mad
ip6table_filter ib_core ip6_tables iscsi_tcp libiscsi_tcp libiscsi
scsi_transport_iscsi dm_multipath coretemp crc32c_intel ghash_clmulni_intel
pcspkr ixgbe joydev mdio igb ptp pps_core dca vhost_net tun macvtap macvlan
uinput raid1 sd_mod i2c_algo_bit drm_kms_helper ttm drm usb_storage mpt2sas
raid_class scsi_transport_sas i2c_core
Pid: 8466, comm: insmod Tainted: GF O 3.9.4 #1
Call Trace:
[] warn_slowpath_common+0x7f/0xc0
[] warn_slowpath_fmt+0x46/0x50
[] ? strlcat+0x65/0x90
[] sysfs_add_one+0xd4/0x100
[] create_dir+0x78/0xd0
[] sysfs_create_dir+0x86/0xe0
[] kobject_add_internal+0xa8/0x270
[] kobject_init_and_add+0x63/0x90
[] load_module+0x12dd/0x2890
[] ? ddebug_proc_open+0xc0/0xc0
[] sys_init_module+0xea/0x140
[] system_call_fastpath+0x16/0x1b
---[ end trace 54bd469258bec620 ]---
[ cut here ]
WARNING: at lib/kobject.c:196 kobject_add_internal+0x204/0x270()
Hardware name: ftServer 6400
kobject_add_internal failed for test_mod with -EEXIST, don't try to register
things with the same name in the same directory.
Modules linked in: test_mod(OF+) ebtable_nat nf_conntrack_netbios_ns
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat_ipv4 nf_nat iptable_mangle
nf_conntrack_ipv4 nf_defrag_ipv4 bonding xt_conntrack nf_conntrack ib_iser
rdma_cm ebtable_filter ib_addr ebtables iw_cm ib_cm ib_sa ib_mad
ip6table_filter ib_core ip6_tables iscsi_tcp libiscsi_tcp libiscsi
scsi_transport_iscsi dm_multipath coretemp crc32c_intel ghash_clmulni_intel
pcspkr ixgbe joydev mdio igb ptp pps_core dca vhost_net tun macvtap macvlan
uinput raid1 sd_mod i2c_algo_bit drm_kms_helper ttm drm usb_storage mpt2sas
raid_class scsi_transport_sas i2c_core
Pid: 8466, comm: insmod Tainted: GF W O 3.9.4 #1
Call Trace:
[] warn_slowpath_common+0x7f/0xc0
[] warn_slowpath_fmt+0x46/0x50
[] kobject_add_internal+0x204/0x270
[] kobject_init_and_add+0x63/0x90
[] load_module+0x12dd/0x2890
[] ? ddebug_proc_open+0xc0/0xc0
[] sys_init_module+0xea/0x140
[] system_call_fastpath+0x16/0x1b
---[ end trace 54bd469258bec621 ]---
test_mod: module is already loaded
test_mod: module is already loaded
BUG: unable to handle kernel paging request at a02ed08c
IP: [] kobject_put+0x11/0x60
PGD 1c0f067 PUD 1c10063 PMD 84dd68067 PTE 0
Oops: [#1] SMP
Modules linked in: test_mod(OF+) ebtable_nat nf_conntrack_netbios_ns
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat_ipv4 nf_nat iptable_mangle
nf_conntrack_ipv4 nf_defrag_ipv4 bonding xt_conntrack nf_conntrack ib_iser
rdma_cm ebtable_filter ib_addr ebtables iw_cm ib_cm ib_sa ib_mad
ip6table_filter ib_core ip6_tables iscsi_tcp libiscsi_tcp libiscsi
scsi_transport_iscsi dm_multipath coretemp crc32c_intel ghash_clmulni_intel
pcspkr ixgbe joydev mdio igb ptp pps_core dca vhost_net tun macvtap macvlan
uinput raid1 sd_mod i2c_algo_bit drm_kms_helper ttm drm usb_storage mpt2sas
raid_class scsi_transport_sas i2c_core
CPU 25
Pid: 8551, comm: insmod Tainted: GF W O 3.9.4 #1 Stratus ftServer
6400/G7LAZ
RIP: 0010:[] [] kobject_put+0x11/0x60
RSP: 0018:881050b95d58 EFLAGS: 00010286
RAX: 0022 RBX: a02ed050 RCX: 88107fd2fba8
RDX: RSI: 88107fd2df58 RDI: a02ed050
RBP: 881050b95d68 R08: 81ce2080 R09: 07c6
R10: R11:
Re: Please add to stable: module: don't unlink the module until we've removed all exposure.
On Sun, 2 Jun 2013, Rusty Russell wrote: > Ben Greear writes: > > > It turns out, the bug I spent yesterday chasing in various 3.9 kernels is > > apparently > > fixed by the commit in the title (c9c390bb5535380d40614571894ef0c00bc026ff). > > Apparently being the operative word. > > This commit avoids the entire "module insert failed due to sysfs race" > path in the common case, it doesn't fix any actual problem. > > I think the real commit you want is Linus' kobject fix > a49b7e82cab0f9b41f483359be83f44fbb6b4979 "kobject: fix kset_find_obj() > race with concurrent last kobject_put()". > > Or is that already in stable? Hi Rusty, I had pointed Ben (offlist) to that bugzilla entry without realizing there were other earlier related fixes in this space. Re-viewing bz- 58011, it looks like it was opened against 3.8.12, while Ben and myself had encountered module loading problems in versions 3.9 and 3.9.[1-3]. I can update the bugzilla entry to add a comment noting commit a49b7e82 "kobject: fix kset_find_obj() race with concurrent last kobject_put()". That said, it doesn't appear that commit 944a1fa "module: don't unlink the module until we've removed all exposure" has not made it into any stable kernel. On my system, applying this on top of 3.9 resolved a module unload/load race that would occasionally occur on boot (two video adapters of the same make, the module unloads for whatever reason and I see "module is already loaded" and "sysfs: cannot create duplicate filename '/module/mgag200'" messages every 5-10% instances.) I have logs if you were interested in these warnings/crashes. Hope this clarifies things. Regards, -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Please add to stable: module: don't unlink the module until we've removed all exposure.
[fixing Cc: sta...@kernel.org address] On Sun, 2 Jun 2013, Joe Lawrence wrote: > On Sun, 2 Jun 2013, Rusty Russell wrote: > > > Ben Greear writes: > > > > > It turns out, the bug I spent yesterday chasing in various 3.9 kernels is > > > apparently > > > fixed by the commit in the title > > > (c9c390bb5535380d40614571894ef0c00bc026ff). > > > > Apparently being the operative word. > > > > This commit avoids the entire "module insert failed due to sysfs race" > > path in the common case, it doesn't fix any actual problem. > > > > I think the real commit you want is Linus' kobject fix > > a49b7e82cab0f9b41f483359be83f44fbb6b4979 "kobject: fix kset_find_obj() > > race with concurrent last kobject_put()". > > > > Or is that already in stable? > > Hi Rusty, > > I had pointed Ben (offlist) to that bugzilla entry without realizing > there were other earlier related fixes in this space. Re-viewing bz- > 58011, it looks like it was opened against 3.8.12, while Ben and myself > had encountered module loading problems in versions 3.9 and > 3.9.[1-3]. I can update the bugzilla entry to add a comment noting commit > a49b7e82 "kobject: fix kset_find_obj() race with concurrent last > kobject_put()". > > That said, it doesn't appear that commit 944a1fa "module: don't unlink the > module until we've removed all exposure" has not made it into any stable > kernel. On my system, applying this on top of 3.9 resolved a module > unload/load race that would occasionally occur on boot (two video adapters > of the same make, the module unloads for whatever reason and I see "module > is already loaded" and "sysfs: cannot create duplicate filename > '/module/mgag200'" messages every 5-10% instances.) I have logs if you > were interested in these warnings/crashes. > > Hope this clarifies things. > > Regards, > > -- Joe > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Please add to stable: module: don't unlink the module until we've removed all exposure.
[Cc: sta...@vger.kernel.org] Third time is a charm? The stable address was incorrect from the first msg in this thread, but the relevant bits remain quoted below... On Mon, 3 Jun 2013, Joe Lawrence wrote: > [fixing Cc: sta...@kernel.org address] > > On Sun, 2 Jun 2013, Joe Lawrence wrote: > > > On Sun, 2 Jun 2013, Rusty Russell wrote: > > > > > Ben Greear writes: > > > > > > > It turns out, the bug I spent yesterday chasing in various 3.9 kernels > > > > is apparently > > > > fixed by the commit in the title > > > > (c9c390bb5535380d40614571894ef0c00bc026ff). > > > > > > Apparently being the operative word. > > > > > > This commit avoids the entire "module insert failed due to sysfs race" > > > path in the common case, it doesn't fix any actual problem. > > > > > > I think the real commit you want is Linus' kobject fix > > > a49b7e82cab0f9b41f483359be83f44fbb6b4979 "kobject: fix kset_find_obj() > > > race with concurrent last kobject_put()". > > > > > > Or is that already in stable? > > > > Hi Rusty, > > > > I had pointed Ben (offlist) to that bugzilla entry without realizing > > there were other earlier related fixes in this space. Re-viewing bz- > > 58011, it looks like it was opened against 3.8.12, while Ben and myself > > had encountered module loading problems in versions 3.9 and > > 3.9.[1-3]. I can update the bugzilla entry to add a comment noting commit > > a49b7e82 "kobject: fix kset_find_obj() race with concurrent last > > kobject_put()". > > > > That said, it doesn't appear that commit 944a1fa "module: don't unlink the > > module until we've removed all exposure" has not made it into any stable > > kernel. On my system, applying this on top of 3.9 resolved a module > > unload/load race that would occasionally occur on boot (two video adapters > > of the same make, the module unloads for whatever reason and I see "module > > is already loaded" and "sysfs: cannot create duplicate filename > > '/module/mgag200'" messages every 5-10% instances.) I have logs if you > > were interested in these warnings/crashes. > > > > Hope this clarifies things. > > > > Regards, > > > > -- Joe > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: __rtc_read_alarm missing month/year field bug?
On 07/19/2016, Alexandre Belloni wrote: > > Well like said in my previous mail, I don't think the rollover is the > issue here but I'm interested in knowing what conditions are leading > to endless interrupts. Hi Alexandre, Unfortunately I've switched employers so I no longer have access to the hardware, but if I remember correctly the scenario went like this: - boot machine with RTC alarm set in far future - run 'hwclock' user program - hwclock sync to clock tick - RTC_UIE_ON - RTC_UIE_OFF - kernel sees RTC alarm is outstanding - hpet_rtc_interrupt's start streaming in In the case of this hardware platform, there was a problem with the CMOS RTC read such that hpet_rtc_interrupt / rtc_cmos_read believed that the RTC was busy updating ... that code would delay long enough that the next HPET rtc interrupt would come in right behind it and choke the CPU from scheduling anything else. That platform-specific bug aside, I thought I would report this strangeness in case it was a real bug -- looks like I didn't fully consider the rollover case. -- Joe
__rtc_read_alarm missing month/year field bug?
Hello Alessandro and Alexandre,
I noticed an interesting cmos_rtc.rtc.aie_timer on a Stratus machine
running the 4.6 kernel, with an expiration time that puts the alarm way
out into next year. This is easily reproducible on this machine by
setting a wakealarm sometime in the near future, then rebooting.
>From a fresh boot:
% cat /proc/driver/rtc
rtc_time: 17:55:10
rtc_date: 2016-06-09
alrm_time : 14:04:37
alrm_date : 2017-06-09 << 2017 ?
alarm_IRQ : no
alrm_pending: no
update IRQ enabled : no
periodic IRQ enabled: no
periodic IRQ frequency : 1024
max user IRQ frequency : 64
24hr: yes
periodic_IRQ: no
update_IRQ : no
HPET_emulated : yes
BCD : yes
DST_enable : no
periodic_freq : 1024
batt_status : okay
I added some debugging code to the kernel, saw this on the next boot:
__rtc_read_alarm: A - alarm->time.tm_year = -1, missing = 0
__rtc_read_alarm: B - alarm->time.tm_year = 116, missing = 3
__rtc_read_alarm: C - alarm->time.tm_year = 117
Corresponding to these parts of __rtc_read_alarm:
int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
...
enum { none, day, month, year } missing = none;
...
err = rtc_read_alarm_internal(rtc, alarm);
...
/* Fill in the missing alarm fields using the timestamp; we
* know there's at least one since alarm->time is invalid.
*/
...
[A]
if (alarm->time.tm_year == -1) {
alarm->time.tm_year = now.tm_year;
if (missing == none)
missing = year;
}
[B]
...
switch (missing) {
...
/* Year rollover ... easy except for leap years! */
case year:
dev_dbg(&rtc->dev, "alarm rollover: %s\n", "year");
do {
alarm->time.tm_year++;
} while (!is_leap_year(alarm->time.tm_year + 1900)
&& rtc_valid_tm(&alarm->time) != 0);
[C] break;
I noticed that the missing year and month cases increment their
respective time units inside a do ... while (condition) loop, pushing
the default 'filled-in' values to now + 1.
Should this 'roll-over' code check for a valid date before incrementing
the alarm time? (See attached patch.) I think this might also apply to
a missing month field as well.
(After the patch + reboot):
% cat /proc/driver/rtc
rtc_time: 18:24:02
rtc_date: 2016-06-09
alrm_time : 14:04:37
alrm_date : 2016-06-09
alarm_IRQ : no
alrm_pending: no
update IRQ enabled : no
periodic IRQ enabled: no
periodic IRQ frequency : 1024
max user IRQ frequency : 64
24hr: yes
periodic_IRQ: no
update_IRQ : no
HPET_emulated : yes
BCD : yes
DST_enable : no
periodic_freq : 1024
batt_status : okay
-- >8 --
>From d6feacf20b312c8ebfee902b8b84f68c1a82f035 Mon Sep 17 00:00:00 2001
From: Joe Lawrence
Date: Thu, 9 Jun 2016 14:52:28 -0400
Subject: [PATCH] rtc: check filled-in alarm values before incrementing
In __rtc_read_alarm, check filled-in alarm->time.tm_year values (those
not returned by the RTC and defaulted to now.tm_year) before
incrementing them in the rollover handling case.
Signed-off-by: Joe Lawrence
---
drivers/rtc/interface.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 9ef5f6f89f98..3098ce4167ef 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -258,10 +258,10 @@ int __rtc_read_alarm(struct rtc_device *rtc,
struct rtc_wkalrm *alarm)
/* Year rollover ... easy except for leap years! */
case year:
dev_dbg(&rtc->dev, "alarm rollover: %s\n", "year");
- do {
+ while (!is_leap_year(alarm->time.tm_year + 1900)
+ && rtc_valid_tm(&alarm->time) != 0) {
alarm->time.tm_year++;
- } while (!is_leap_year(alarm->time.tm_year + 1900)
- && rtc_valid_tm(&alarm->time) != 0);
+ }
break;
default:
--
1.8.3.1
Re: [PATCH v4] livepatch: introduce shadow variable API
On 08/16/2017 08:43 AM, Miroslav Benes wrote:
>
>> [ ... snip ... ]
>
> There is a comment above about locking and we do not take the spinlock
> here. That could surprise someone. So I'd keep only klp_shadow_add()
> comment, because there it is strictly needed. It depends on the context in
> all other cases.
Good catch, I think this changed in this last version when I moved some
of the work outside the lock.
> Could you also add a comment above klp_shadow_lock definition about what
> it aims to protect?
>
How about "klp_shadow_lock provides exclusive access to the
klp_shadow_hash and the shadow variables it references." or were
thinking of something more detailed?
>> +/* Look for again under the lock */
>> +spin_lock_irqsave(&klp_shadow_lock, flags);
>> +shadow_data = klp_shadow_get(obj, id);
>> +if (unlikely(shadow_data)) {
>
> shadow_data is not needed anywhere, so you could do the same as for the
> first speculative search and remove shadow_data variable all together.
Ok.
>> [ ... snip ... ]
>
> Otherwise it looks good. You can add my
>
> Acked-by: Miroslav Benes
>
> with those nits fixed.
Thank you for all the suggestions and reviews!
-- Joe
[PATCH v3] add (un)patch callbacks
patching complete [ 79.711756] livepatch_callbacks_demo: post_patch_callback: vmlinux [ 80.859474] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled [ 80.860441] livepatch: 'livepatch_callbacks_demo': unpatching... [ 81.759137] livepatch: 'livepatch_callbacks_demo': unpatching complete [ 81.760137] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux [ 81.760994] livepatch_callbacks_demo: post_unpatch_callback: vmlinux [ 82.862596] % rmmod samples/livepatch/livepatch-callbacks-demo.ko Part 1: livepatch is loaded (no target module), only vmlinux callbacks run. Part 2: livepatch is disabled (no target module), only vmlinux callbacks run. [ 84.878971] -- test6 - load target module, load livepatch -ENODEV, unload target module [ 84.879755] % insmod samples/livepatch/livepatch-callbacks-mod.ko [ 84.882842] livepatch_callbacks_mod: livepatch_callbacks_mod_init [ 86.885313] % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19 [ 86.889259] livepatch: enabling patch 'livepatch_callbacks_demo' [ 86.890160] livepatch_callbacks_demo: pre_patch_callback: vmlinux [ 86.890734] livepatch: pre-patch callback failed for object 'vmlinux' [ 86.891306] livepatch: failed to enable patch 'livepatch_callbacks_demo' [ 86.891931] livepatch_callbacks_demo: post_unpatch_callback: vmlinux [ 86.892561] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state [ 86.908817] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device [ 88.911655] % rmmod samples/livepatch/livepatch-callbacks-demo.ko [ 88.914815] rmmod: ERROR: Module livepatch_callbacks_demo is not currently loaded [ 90.917163] % rmmod samples/livepatch/livepatch-callbacks-mod.ko [ 90.919997] livepatch_callbacks_mod: livepatch_callbacks_mod_exit Part 1: Livepatch is loaded after the target module, however the vmlinux-pre-patch-callback returns -ENODEV, so the livepatch module fails to load. Note: both vmlinux and target module's post-unpatch-callbacks are executed as part of the cancelled transition: klp_enable_patch __klp_enable_patch klp_cancel_transition klp_complete_transition done: ... else if (klp_target_state == KLP_UNPATCHED) klp_post_unpatch_callback(obj); [ 92.934851] -- test7 - load livepatch, setup -ENODEV, load target module, disable livepatch, unload livepatch [ 92.935879] % insmod samples/livepatch/livepatch-callbacks-demo.ko [ 92.938683] livepatch: enabling patch 'livepatch_callbacks_demo' [ 92.939294] livepatch_callbacks_demo: pre_patch_callback: vmlinux [ 92.939823] livepatch: 'livepatch_callbacks_demo': patching... [ 93.727126] livepatch: 'livepatch_callbacks_demo': patching complete [ 93.727809] livepatch_callbacks_demo: post_patch_callback: vmlinux [ 94.942396] % echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret [ 96.944893] % insmod samples/livepatch/livepatch-callbacks-mod.ko [ 96.947557] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod' [ 96.948816] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init [ 96.950416] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod' [ 96.951424] livepatch: patch 'livepatch_callbacks_demo' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod' [ 96.966586] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device [ 98.968923] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled [ 98.970179] livepatch: 'livepatch_callbacks_demo': unpatching... [ 100.703101] livepatch: 'livepatch_callbacks_demo': unpatching complete [ 100.704057] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux [ 100.704898] livepatch_callbacks_demo: post_unpatch_callback: vmlinux [ 100.972541] % rmmod samples/livepatch/livepatch-callbacks-mod.ko [ 100.975462] rmmod: ERROR: Module livepatch_callbacks_mod is not currently loaded [ 102.977392] % rmmod samples/livepatch/livepatch-callbacks-demo.ko Part 1: Livepatch is loaded first, so vmlinux callbacks run Part 2: The livepatch's pre-patch-callback is setup to now return -ENODEV Part 3: When a targetted module is loaded, the pre-patch-callback returns -ENODEV and the target module fails to load. Part 4: The livepatch is disabled and only the vmlinux unpatch-callbacks are executed. Note: this test should be consistent with the other test which fails a pre-patch-callback status... ie, the post-unpatch-callback behavior for said klp_object should be the same. -- Joe Lawrence (1): livepatch: add (un)patch callbacks Documentation/livepatch/callbacks.txt| 87 include/linux/livepatch.h| 81 kernel/livepatch/core.c | 37 -- kernel/livepatch/patch.c | 5 +- kernel/livepatch/transition.c| 21 ++- samples/livepatch/Makefile | 2 + samples/livepatch/livepatch-callbacks-demo.c | 190 +++ samples/livepatch/livepatch-callbacks-mod.c | 53 8 files changed, 462 insertions(+), 14 deletions(-) create mode 100644 Documentation/livepatch/callbacks.txt create mode 100644 samples/livepatch/livepatch-callbacks-demo.c create mode 100644 samples/livepatch/livepatch-callbacks-mod.c -- 1.8.3.1
[PATCH v3] livepatch: add (un)patch callbacks
Provide livepatch modules a klp_object (un)patching notification
mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
setup or synchronize changes that would be difficult to support in only
patched-or-unpatched code contexts.
Callbacks can be registered for target module or vmlinux klp_objects,
but each implementation is klp_object specific.
- Pre-(un)patch callbacks run before any (un)patching action takes
place.
- Post-(un)patch callbacks run once an object has been (un)patched and
the klp_patch fully transitioned to its target state.
Example use cases include modification of global data and registration
of newly available services/handlers.
See Documentation/livepatch/callback.txt for details.
Signed-off-by: Joe Lawrence
---
Documentation/livepatch/callbacks.txt| 87
include/linux/livepatch.h| 81
kernel/livepatch/core.c | 37 --
kernel/livepatch/patch.c | 5 +-
kernel/livepatch/transition.c| 21 ++-
samples/livepatch/Makefile | 2 +
samples/livepatch/livepatch-callbacks-demo.c | 190 +++
samples/livepatch/livepatch-callbacks-mod.c | 53
8 files changed, 462 insertions(+), 14 deletions(-)
create mode 100644 Documentation/livepatch/callbacks.txt
create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
create mode 100644 samples/livepatch/livepatch-callbacks-mod.c
diff --git a/Documentation/livepatch/callbacks.txt
b/Documentation/livepatch/callbacks.txt
new file mode 100644
index ..e18f2678f3bb
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,87 @@
+(Un)patching Callbacks
+==
+
+Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
+to execute callback functions when a kernel object is (un)patched. They
+can be considered a "power feature" that extends livepatching abilities
+to include:
+
+ - Safe updates to global data
+
+ - "Patches" to init and probe functions
+
+ - Patching otherwise unpatchable code (i.e. assembly)
+
+In most cases, (un)patch hooks will need to be used in conjunction with
+memory barriers and kernel synchronization primitives, like
+mutexes/spinlocks, or even stop_machine(), to avoid concurrency issues.
+
+Callbacks differ from existing kernel facilities:
+
+ - Module init/exit code doesn't run when disabling and re-enabling a
+patch.
+
+ - A module notifier can't stop the to-be-patched module from loading.
+
+Callbacks are part of the klp_object structure and their implementation
+is specific to the given object. Other livepatch objects may or may not
+be patched, irrespective of the target klp_object's current state.
+
+Callbacks can be registered for the following livepatch actions:
+
+ * Pre-patch- before klp_object is patched
+
+ * Post-patch - after klp_object has been patched and is active
+ across all tasks
+
+ * Pre-unpatch - before klp_object is unpatched, patched code is active
+
+ * Post-unpatch - after klp_object has been patched, all code has been
+ restored and no tasks are running patched code
+
+A callbacks is only executed if its host klp_object is loaded. For
+in-kernel vmlinux targets, this means that callbacks will always execute
+when a livepatch is enabled/disabled.
+
+For kernel module targets, callbacks will only execute if the target
+module is loaded. When a kernel module target is (un)loaded, its
+callbacks will execute only if the livepatch module is enabled.
+
+The pre-patch callback is expected to return a status code (0 for
+success, -ERRNO on error). An error status code will indicate to the
+livepatching core that patching of the current klp_object is not safe
+and to stop the current patching request. If the problematic klp_object
+is already loaded (i.e. a livepatch is loaded after target code), the
+kernel's module loader will refuse to load the livepatch. On the other
+hand, if the problematic klp_object is already in place (i.e. a target
+module is loaded after a livepatch), then the module loader will refuse
+to load the target kernel module.
+
+
+Example Use-cases
+-
+
+1 - Update global data
+
+A pre-patch callback can be useful to update a global variable. For
+example, 75ff39ccc1bd ("tcp: make challenge acks less predictable")
+changes a global sysctl, as well as patches the tcp_send_challenge_ack()
+function.
+
+In this case, if we're being super paranoid, it might make sense to
+patch the data *after* patching is complete with a post-patch callback,
+so that tcp_send_challenge_ack() could first be changed to read
+sysctl_tcp_challenge_ack_limit with READ_ONCE.
+
+
+2 - Support __init and probe function patches
+
+Although __init and probe functions are not directly livepatch-able, it
+may be possible to
Re: [PATCH v4] livepatch: introduce shadow variable API
On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
>> Add exported API for livepatch modules:
>>
>> klp_shadow_get()
>> klp_shadow_attach()
>> klp_shadow_get_or_attach()
>> klp_shadow_update_or_attach()
>> klp_shadow_detach()
>> klp_shadow_detach_all()
>>
>> that implement "shadow" variables, which allow callers to associate new
>> shadow fields to existing data structures. This is intended to be used
>> by livepatch modules seeking to emulate additions to data structure
>> definitions.
>>
>> See Documentation/livepatch/shadow-vars.txt for a summary of the new
>> shadow variable API, including a few common use cases.
>>
>> See samples/livepatch/livepatch-shadow-* for example modules that
>> demonstrate shadow variables.
>>
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> new file mode 100644
>> index ..0ebd4b635e4f
>> --- /dev/null
>> +++ b/kernel/livepatch/shadow.c
>> +/**
>> + * klp_shadow_match() - verify a shadow variable matches given
>> + * @shadow: shadow variable to match
>> + * @obj:pointer to parent object
>> + * @id: data identifier
>> + *
>> + * Return: true if the shadow variable matches.
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
>> +unsigned long id)
>> +{
>> +return shadow->obj == obj && shadow->id == id;
>> +}
>
> Do we really need this function? It is called only in situations
> where shadow->obj == obj is always true. Especially the use in
> klp_shadow_detach_all() is funny because we pass shadow->obj as
> the shadow parameter.
Personal preference. Abstracting out all of the routines that operated
on the shadow variables (setting up, comparison) did save some code
lines and centralized these common bits.
>> +
>> +/**
>> + * klp_shadow_get() - retrieve a shadow variable data pointer
>> + * @obj:pointer to parent object
>> + * @id: data identifier
>> + *
>> + * Return: the shadow variable data element, NULL on failure.
>> + */
>> +void *klp_shadow_get(void *obj, unsigned long id)
>> +{
>> +struct klp_shadow *shadow;
>> +
>> +rcu_read_lock();
>> +
>> +hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
>> + (unsigned long)obj) {
>> +
>> +if (klp_shadow_match(shadow, obj, id)) {
>> +rcu_read_unlock();
>> +return shadow->data;
>> +}
>> +}
>> +
>> +rcu_read_unlock();
>> +
>> +return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_get);
>> +
>> +/*
>> + * klp_shadow_set() - initialize a shadow variable
>> + * @shadow: shadow variable to initialize
>> + * @obj:pointer to parent object
>> + * @id: data identifier
>> + * @data: pointer to data to attach to parent
>> + * @size: size of attached data
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
>> + unsigned long id, void *data, size_t size)
>> +{
>> +shadow->obj = obj;
>> +shadow->id = id;
>> +
>> +if (data)
>> +memcpy(shadow->data, data, size);
>> +}
>
> The function name suggests that it is a counterpart of
> klp_shadow_get() but it is not. Which is a bit confusing.
I should have called it "setup" or "init", but perhaps that's moot ...
> Hmm, the purpose of this function is to reduce the size of cut&pasted
> code between all that klp_shadow_*attach() variants. But there
> is still too much cut&pasted code. In fact, the base logic of all
> variants is basically the same. The only small difference should be
> how they handle the situation when the variable is already there.
... this is true. An earlier draft revision that I had discarded
attempted combining all cases. I had used two extra function arguments,
"update" and "duplicates", to key off for each behavior... it turned
into a complicated, full-screen page of conditional logic, so I threw it
out.
However, I like the way you pulled it off using a jump-to-switch
statement at the bottom of the function...
> OK, there is a locking difference in the update variant but
> it is questionable, se
Re: [PATCH v3 0/4] A few round_pipe_size() and pipe-max-size fixups
On 10/10/2017 02:04 PM, Joe Lawrence wrote: > While backporting Michael's "pipe: fix limit handling" patchset to a > distro-kernel, Mikulas noticed that current upstream pipe limit handling > contains a few problems: > > 1 - procfs signed wrap: echo'ing a large number into > /proc/sys/fs/pipe-max-size and then cat'ing it back out shows a > negative value. > > 2 - round_pipe_size() nr_pages overflow on 32bit: this would > subsequently try roundup_pow_of_two(0), which is undefined. > > 3 - visible non-rounded pipe-max-size value: there is no mutual > exclusion or protection between the time pipe_max_size is assigned > a raw value from proc_dointvec_minmax() and when it is rounded. > > 4 - unsigned long -> unsigned int conversion makes for potential odd > return errors from do_proc_douintvec_minmax_conv() and > do_proc_dopipe_max_size_conv(). > > This version underwent the same testing as v1: > https://marc.info/?l=linux-kernel&m=150643571406022&w=2 > > v1 (from rfc): > > - Re-arrange patchset order, push smaller fixes to the front > - Add a check so that round_pipe_size(size < pipe_min_size) will round > up to round_pipe_size(pipe_min_size) as per man page [RD] > - Add new procfs proc_dopipe_max_size() and helpers to consolidate user > space read / type validation / rounding / assignment [MP] > > v2: > - Fix !CONFIG_PROC_SYSCTL build case [buildbots] > > v3: > - s/proc_dointvec_minmax/proc_dopipe_max_size/ in comment >preceding pipe_proc_fn() > - Added a fourth patch to address -ERANGE vs. -EINVAL inconsistencies in >do_proc_douintvec_minmax_conv() and do_proc_dopipe_max_size_conv(). > > Joe Lawrence (4): > pipe: match pipe_max_size data type with procfs > pipe: avoid round_pipe_size() nr_pages overflow on 32-bit > pipe: add proc_dopipe_max_size() to safely assign pipe_max_size > sysctl: check for UINT_MAX before unsigned int min/max > > fs/pipe.c | 23 ++- > include/linux/pipe_fs_i.h | 1 + > include/linux/sysctl.h| 3 +++ > kernel/sysctl.c | 57 > --- > 4 files changed, 70 insertions(+), 14 deletions(-) > Thanks for the reviews, now who do I need to bug to get this merged into a tree? :) v1: Reviewed-by: Michael Kerrisk "Looks good" from Randy Dunlap v3: Reviewed-by: Mikulas Patocka -- Joe
Re: [PATCH 2/2] livepatch: __klp_disable_patch() should never be called for disabled patches
On Fri, Oct 20, 2017 at 04:56:51PM +0200, Petr Mladek wrote:
> __klp_disable_patch() should never be called when the patch is not
> enabled. Let's add the same warning that we have in __klp_enable_patch().
>
> This allows to remove the check when calling klp_pre_unpatch_callback().
> It was strange anyway because it repeatedly checked per-patch flag
> for each patched object.
>
> Signed-off-by: Petr Mladek
> ---
> kernel/livepatch/core.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb134479c394..287f71e9dbfe 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -282,6 +282,9 @@ static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
>
> + if (WARN_ON(!patch->enabled))
> + return -EINVAL;
> +
> if (klp_transition_patch)
> return -EBUSY;
>
> @@ -293,7 +296,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
> klp_init_transition(patch, KLP_UNPATCHED);
>
> klp_for_each_object(patch, obj)
> - if (patch->enabled && obj->patched)
> + if (obj->patched)
> klp_pre_unpatch_callback(obj);
>
> /*
> --
> 1.8.5.6
Looks reasonable to me and cleans up the klp_pre_unpatch_callback()
calling condition.
Acked-by: Joe Lawrence
Re: [PATCH 1/2] livepatch: Correctly call klp_post_unpatch_callback() in error paths
On Fri, Oct 20, 2017 at 04:56:50PM +0200, Petr Mladek wrote:
>
Hi Petr,
Thank you for these fixups! I ran them through the tests
in Documentation/livepatch/callbacks.txt just to be safe and it looks
good. I wish I had added a few tests for klp_patch_object() failures,
but I would have needed to somehow force those errors. At that point
I'd have more of a regression test than a human readable document.
Anyway, if this set can be squashed into for-4.15/callbacks, then great.
If not, then may I suggest a few rewordings/cleanups for this commit
message:
> The flag post_unpatch_enabled in struct klp_callbacks is need in
> the error paths. We need to call the post_unpatch callback
> only when the pre_patch one was called.
>
> We should clear the flag in klp_post_unpatch_callback() to make
> sure that the callback is not called twice. It makes the API
> more safe.
The post_unpatch_enabled flag in struct klp_callbacks is set when a
pre-patch callback successfully executes, indicating that we need to
call a corresponding post-unpatch callback when the patch is reverted.
This is true for ordinary patch disable as well as the error paths of
klp_patch_object() callers.
> Note that we actually would call the callback twice in
> klp_module_coming() when klp_patch_object() fails.
> In this case, we explicitly call klp_post_unpatch_callback()
> for the failed object. And we are going to call it once
> again when reverting operations for all the patches by
> reusing the klp_module_going() code. There is a patch
> doing this in the queue.
As currently coded, we inadvertently execute the post-patch callback
twice in klp_module_coming() when klp_patch_object() fails:
- We explicitly call klp_post_unpatch_callback() for the failed object
- We call it again for the same object (and all the others) via
klp_cleanup_module_patches_limited()
We should clear the flag in klp_post_unpatch_callback() to make
sure that the callback is not called twice. It makes the API
more safe.
(We could have removed the callback from the former error path as it
would be covered by the latter call, but I think that is is cleaner to
clear the post_unpatch_enabled after its invoked. For example, someone
might later decide to call the callback only when obj->patched flag is
set.)
> There was another mistake in the error path in klp_comming_module().
> It called klp_post_unpatch_callback() only when
> patch != klp_transition_patch. But klp_pre_patch_callback()
> was called even for this patch.
There is another mistake in the error path of klp_coming_module() in
which it skips the post-unpatch callback for the klp_transition_patch.
However, the pre-patch callback was called even for this patch, so be
sure to make the corresponding callbacks for all patches.
> In fact, we could remove klp_post_unpatch_callback() from
> the error path at all because it will be covered by
> the reuse of the klp_module_going() code. But I think
> that it is cleaner this way. For example, someone
> might later decide to call the callback only when
> obj->patched flag is set.
JL: (Moved up above.)
>
> Finally, I used this opportunity to make klp_pre_patch_callback()
> more readable.
JL: Agreed that this is clearer to read, thanks.
>
> Signed-off-by: Petr Mladek
Acked-by: Joe Lawrence
> ---
> kernel/livepatch/core.c | 4 +---
> kernel/livepatch/core.h | 8 +---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index cafb5a84417d..eb134479c394 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -894,9 +894,7 @@ int klp_module_coming(struct module *mod)
> pr_warn("failed to apply patch '%s' to module
> '%s' (%d)\n",
> patch->mod->name, obj->mod->name, ret);
>
> - if (patch != klp_transition_patch)
> - klp_post_unpatch_callback(obj);
> -
> + klp_post_unpatch_callback(obj);
> goto err;
> }
>
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 6fc907b54e71..cc3aa708e0b4 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -12,10 +12,10 @@ static inline bool klp_is_object_loaded(struct klp_object
> *obj)
>
> static inline int klp_pre_patch_callback(struct klp_object *obj)
> {
> - int ret;
> + int ret = 0;
>
> - ret = (obj->callbacks.pre_patch) ?
> - (*obj->callbacks.pre_patch)(obj) : 0;
> + if (obj->callbacks.pre_patch)
> + ret = (*obj->callbacks.pre_patch)(obj);
>
> obj-&
Re: [PATCH v2 0/7] pipe: buffer limits fixes and cleanups
On 01/11/2018 12:28 AM, Eric Biggers wrote: > This series simplifies the sysctl handler for pipe-max-size and fixes > another set of bugs related to the pipe buffer limits: > > - The root user wasn't allowed to exceed the limits when creating new > pipes. > > - There was an off-by-one error when checking the limits, so a limit of > N was actually treated as N - 1. > > - F_SETPIPE_SZ accepted values over UINT_MAX. > > - Reading the pipe buffer limits could be racy. > > Changed since v1: > > - Added "Fixes" tag to "pipe: fix off-by-one error when checking buffer > limits" > - In pipe_set_size(), checked 'nr_pages' rather than 'size' > - Fixed commit message for "pipe: simplify round_pipe_size()" > > Eric Biggers (7): > pipe, sysctl: drop 'min' parameter from pipe-max-size converter > pipe, sysctl: remove pipe_proc_fn() > pipe: actually allow root to exceed the pipe buffer limits > pipe: fix off-by-one error when checking buffer limits > pipe: reject F_SETPIPE_SZ with size over UINT_MAX > pipe: simplify round_pipe_size() > pipe: read buffer limits atomically > > fs/pipe.c | 57 > --- > include/linux/pipe_fs_i.h | 5 ++--- > include/linux/sysctl.h| 3 --- > kernel/sysctl.c | 33 +------ > 4 files changed, 32 insertions(+), 66 deletions(-) > Bug fixes look good and thanks for continuing the code cleanup! For the series: Acked-by: Joe Lawrence -- Joe
Re: [PATCH v5 0/3] livepatch: introduce atomic replace
On 01/30/2018 01:19 PM, Jason Baron wrote: > [ ... snip ... ] > > Our main interest in 'atomic replace' is simply to make sure that > cumulative patches work as expected in that they 'replace' any prior > patches. We have an interest primarily in being able to apply patches > from the stable trees via livepatch. I think the current situation with > respect to 'replace' and callbacks is fine for us as well, but could > change based on more experience with livepatch. Well the callback implementation was based somewhat on theoretical usage.. it was inspired by the kpatch macros that you talk about below, in which we had a few specific use-cases. Converting (un)patch notifiers to the livepatch model presented additional callback locations, and as such we ended up with pre-patch, post-patch, pre-unpatch and post-unpatch callbacks. Hopefully we'll get a better idea of their worth as we gain experience using them. At this point in time I would suggest keeping it as simple and safe as possible. :) > As an aside I was just wondering how you are making use of the callbacks > using the tool you mentioned (that is based on kpatch)? I see in the > upstream kpatch that there are hooks such as: 'KPATCH_LOAD_HOOK(fn)' and > 'KPATCH_UNLOAD_HOOK(fn)'. However, these are specific to 'kpatch' (as > opposed to livepatch), and I do not see these sort of macros for the > recently introduced livepatch callbacks. It seems it would be easy > enough to add similar hooks for the livepatch callbacks. I was thinking > of writing such a patch, but was wondering if there was an existing > solution? I've got a work in progress PR for this very feature: https://github.com/dynup/kpatch/pull/780 Evgenii has already provided some helpful feedback. Another set of eyes/testing would be appreciated! Regards, -- Joe
Re: [PATCH v5 0/3] livepatch: introduce atomic replace
On 01/30/2018 09:03 AM, Petr Mladek wrote: > On Fri 2018-01-26 14:29:36, Evgenii Shatokhin wrote: >> >> In my experience, it was quite convenient sometimes to just "replace all >> binary patches the user currently has loaded with this single one". No >> matter what these original binary patches did and where they came from. > > To be honest, I would feel better if the livepatch framework is > more safe. It should prevent breaking the system by a patch > that atomically replaces other random patches that rely on callbacks. > > Well, combining random livepatches from random vendors is a call > for troubles. It might easily fail when two patches add > different changes to the same function. > > I wonder if we should introduce some tags, keys, vendors. I mean > to define an identification or dependencies that would say that some > patches are compatible or incompatible. > > We could allow to livepatch one function by two livepatches > only if they are from the same vendor. But then the order still > might be important. Also I am not sure if this condition is safe > enough. > > One the other hand, we could handle callbacks like the shadow > variables. Every shadow variable has an ID. We have an API to > add/read/update/remove them. We might allow to have more > callbacks with different IDs. Then we could run callbacks > only for IDs that are newly added or removed. Sigh, this would > be very complex implementation as well. But it might make > these features easier to use and maintain. Interesting ideas, but I wonder if this could be accomplished at the livepatch module level? ie, leave it to kpatch-build (or a livepatch equivalent) to produce a module that does this automatically. I guess it would then be completely opt-in checking, but transfers the complexity out of the kernel livepatching core. I don't see a simple way to provide flexibility of when/if calling redundant callbacks without making the code even more complicated. I don't have any use-cases off hand that would require such features, but I guess if I did absolutely needed them, I might be inclined to say prepare ahead of time and write callbacks so that they may be disabled externally -- either by a new livepatch module's init() or some other means. Then whatever crazy policy I need is contained to my modules, not provided or enforced by the kernel. -- Joe
Re: PATCH v6 2/6] livepatch: Free only structures with initialized kobject
On 01/25/2018 11:01 AM, Petr Mladek wrote: > We are going to add a feature called atomic replace. It will allow to > create a patch that would replace all already registered patches. > For this, we will need to dynamically create funcs' and objects' > for functions that are not longer patched. Super small nit: funcs', objects' ? More on this in the next inline comment. > [ ... snip ... ] > /* > - * Free all functions' kobjects in the array up to some limit. When limit is > - * NULL, all kobjects are freed. > + * Free all funcs' that have the kobject initialized. Otherwise, > + * nothing is needed. In new comment, what is the apostrophe in funcs' for? The code (still) deals with the kobject embedded in the klp_func, but the comment text moved "kobject" such that I think the apostrophe can now be removed. > [ ... snip ... ] > /* > - * Free all objects' kobjects in the array up to some limit. When limit is > - * NULL, all kobjects are freed. > + * Free all funcs' and objects's that have the kobject initialized. > + * Otherwise, nothing is needed. > */ Ditto. -- Joe
Re: PATCH v6 0/6] livepatch: Atomic replace feature
On 02/01/2018 08:49 AM, Miroslav Benes wrote: > > Well, one more thing. I think there is a problem with shadow variables. > Similar to callbacks situation. Shadow variables cannot be destroyed the > way it is shown in our samples. Cumulative patches want to preserve > everything as much as possible. If I'm right, it should be mentioned in > the documentation. Are you talking about using klp_shadow_free_all() call in a module_exit routine? Yeah, I think in this case, that responsibility would be passed to the newly loaded cumulative patch, right? -- Joe
Re: [PATCH v10 00/10] livepatch: Atomic replace feature
Hi Petr,
These are the callback tests that I hacked up into a livepatch
kselftest. (Basically I copied a bunch of the sample modules and
verified the expected dmesg output as I had listed in in the
Documentation/livepatch/callbacks.txt file.) The script is still a
little rough and maybe this isn't the direction we want to go for proper
kselftests, but perhaps it saves you some time/sanity for verifying this
patchset.
Hope this helps,
-- Joe
-- >8 -- >8 -- >8 -- >8 --
>From 0364430c53e12e21923bed20cb651374b4cf9ba9 Mon Sep 17 00:00:00 2001
From: Joe Lawrence
Date: Tue, 6 Mar 2018 17:32:25 -0500
Subject: WIP - livepatch kselftest
CONFIG_TEST_LIVEPATCH=m
% make -C tools/testing/selftests TARGETS=livepatch run_tests
Signed-off-by: Joe Lawrence
---
lib/Kconfig.debug| 11 +
lib/Makefile | 9 +
lib/test_klp_callbacks_busy.c| 58 ++
lib/test_klp_callbacks_demo.c| 205 +++
lib/test_klp_callbacks_demo2.c | 149 +
lib/test_klp_callbacks_mod.c | 39 ++
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/livepatch/Makefile | 5 +
tools/testing/selftests/livepatch/config | 1 +
tools/testing/selftests/livepatch/livepatch-test | 658 +++
10 files changed, 1136 insertions(+)
create mode 100644 lib/test_klp_callbacks_busy.c
create mode 100644 lib/test_klp_callbacks_demo.c
create mode 100644 lib/test_klp_callbacks_demo2.c
create mode 100644 lib/test_klp_callbacks_mod.c
create mode 100644 tools/testing/selftests/livepatch/Makefile
create mode 100644 tools/testing/selftests/livepatch/config
create mode 100755 tools/testing/selftests/livepatch/livepatch-test
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64155e310a9f..cd2a2d25314e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1932,6 +1932,17 @@ config TEST_DEBUG_VIRTUAL
If unsure, say N.
+config TEST_LIVEPATCH
+ tristate "Test livepatching"
+ default n
+ depends on LIVEPATCH
+ help
+ Test various kernel livepatching features for correctness.
+ The tests will load test modules that will be livepatched
+ in various scenarios.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index a90d4fcd748f..919de0acf1a8 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -67,6 +67,15 @@ obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
+obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_callbacks_demo.o \
+ test_klp_callbacks_demo2.o \
+ test_klp_callbacks_busy.o \
+ test_klp_callbacks_mod.o
+CFLAGS_test_klp_callbacks_demo.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_demo2.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_mod.o+= $(CC_FLAGS_FTRACE)
+
ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
CFLAGS_kobject_uevent.o += -DDEBUG
diff --git a/lib/test_klp_callbacks_busy.c b/lib/test_klp_callbacks_busy.c
new file mode 100644
index ..f76a7e6bed00
--- /dev/null
+++ b/lib/test_klp_callbacks_busy.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence
+
+/*
+ * livepatch-callbacks-busymod.c - (un)patching callbacks demo support module
+ *
+ *
+ * Purpose
+ * ---
+ *
+ * Simple module to demonstrate livepatch (un)patching callbacks.
+ *
+ *
+ * Usage
+ * -
+ *
+ * This module is not intended to be standalone. See the "Usage"
+ * section of livepatch-callbacks-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+
+static int sleep_secs;
+module_param(sleep_secs, int, 0644);
+MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)");
+
+static void busymod_work_func(struct work_struct *work);
+static DECLARE_DELAYED_WORK(work, busymod_work_func);
+
+static void busymod_work_func(struct work_struct *work)
+{
+ pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs);
+ msleep(sleep_secs * 1000);
+ pr_info("%s exit\n", __func__);
+}
+
+static int livepatch_callbacks_mod_init(void)
+{
+ pr_info("%s\n", __func__);
+ schedule_delayed_work(&work,
+ msecs_to_jiffies(1000 * 0));
+ return 0;
+}
+
+static void livepatch_callbacks_mod_exit(void)
+{
+ cancel_delayed_work_sync(&work);
+ pr_info("%s\n", __func__);
+}
+
+module_init(livepatch_callbacks_mod_init);
+module_exit(livepatch_callbacks_mod_exit);
+MODULE_LICENSE("GPL");
diff --git a/lib/te
Re: [PATCH v10 00/10] livepatch: Atomic replace feature
On 03/07/2018 03:20 AM, Petr Mladek wrote: > The atomic replace allows to create cumulative patches. They > are useful when you maintain many livepatches and want to remove > one that is lower on the stack. In addition it is very useful when > more patches touch the same function and there are dependencies > between them. > > > Changes against v9: > > + Fixed check of valid NOPs for already loaded objects, > regression introduced in v9 [Joe, Mirek] > + Allow to replace even disabled patches [Evgenii] > > Changes against v8: > > + Fixed handling of statically defined struct klp_object > with empty array of functions [Joe, Mirek] > + Removed redundant func->new_func assignment for NOPs [Mirek] > + Improved some wording [Mirek] > > [ ... snip ... ] Hi Petr, I tried updating the test cases I was adding in "[PATCH v0 0/3] additional cumulative livepatch doc/samples" and although one of the cases is better than before, I'm running into a new issue: an expected pre-unpatch callback is not executed (its obj->patched is false). Here's the updated test case: Test 11 --- - load livepatch - load second livepatch (atomic replace) <- callbacks ok - disable second livepatch <- pre-unpatch skipped - unload livepatch - unload second livepatch % insmod samples/livepatch/livepatch-callbacks-demo.ko [ 2306.806046] livepatch: enabling patch 'livepatch_callbacks_demo' [ 2306.806048] livepatch: 'livepatch_callbacks_demo': initializing patching transition [ 2306.806083] livepatch_callbacks_demo: pre_patch_callback: vmlinux [ 2306.806083] livepatch: 'livepatch_callbacks_demo': starting patching transition [ 2307.743170] livepatch: 'livepatch_callbacks_demo': completing patching transition [ 2307.743317] livepatch_callbacks_demo: post_patch_callback: vmlinux [ 2307.743319] livepatch: 'livepatch_callbacks_demo': patching complet % insmod samples/livepatch/livepatch-callbacks-demo2.ko replace=1 [ 2316.161804] livepatch: enabling patch 'livepatch_callbacks_demo2' [ 2316.161807] livepatch: 'livepatch_callbacks_demo2': initializing patching transition [ 2316.161842] livepatch_callbacks_demo2: pre_patch_callback: vmlinux [ 2316.161843] livepatch: 'livepatch_callbacks_demo2': starting patching transition [ 2317.727141] livepatch: 'livepatch_callbacks_demo2': completing patching transition [ 2317.727254] livepatch_callbacks_demo2: post_patch_callback: vmlinux [ 2317.727255] livepatch: 'livepatch_callbacks_demo2': patching complete % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled [ 2328.995854] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition [ 2328.995898] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition [ 2330.719234] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition [ 2330.719597] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux [ 2330.719599] livepatch: 'livepatch_callbacks_demo2': unpatching complete % rmmod samples/livepatch/livepatch-callbacks-demo2.ko % rmmod samples/livepatch/livepatch-callbacks-demo.ko Running against v10, callbacks seem to be good up until I disable an atomic replace patch. My understanding is that the original patch's unpatch callbacks should be skipped (as they were). I was surprised to see that atomic replacement patch only ran it's post-unpatch callback. Unfortunately I'm running out of time to further debug today, but thought I would share these results. I can dig in more tomorrow. Regards, -- Joe
Re: [PATCH v10 00/10] livepatch: Atomic replace feature
On 03/26/2018 06:56 AM, Petr Mladek wrote: > On Mon 2018-03-12 14:57:04, Joe Lawrence wrote: >> Hi Petr, >> >> These are the callback tests that I hacked up into a livepatch >> kselftest. (Basically I copied a bunch of the sample modules and >> verified the expected dmesg output as I had listed in in the >> Documentation/livepatch/callbacks.txt file.) The script is still a >> little rough and maybe this isn't the direction we want to go for proper >> kselftests, but perhaps it saves you some time/sanity for verifying this >> patchset. > > > >> Hope this helps, >> >> -- Joe >> >> -- >8 -- >8 -- >8 -- >8 -- >> >> >From 0364430c53e12e21923bed20cb651374b4cf9ba9 Mon Sep 17 00:00:00 2001 >> From: Joe Lawrence >> Date: Tue, 6 Mar 2018 17:32:25 -0500 >> Subject: WIP - livepatch kselftest >> >> CONFIG_TEST_LIVEPATCH=m >> % make -C tools/testing/selftests TARGETS=livepatch run_tests >> >> Signed-off-by: Joe Lawrence >> --- >> lib/Kconfig.debug| 11 + >> lib/Makefile | 9 + >> lib/test_klp_callbacks_busy.c| 58 ++ >> lib/test_klp_callbacks_demo.c| 205 +++ >> lib/test_klp_callbacks_demo2.c | 149 + >> lib/test_klp_callbacks_mod.c | 39 ++ > > I would suggest to put it into lib/livepatch/ subdirectory. I hope > that we will add more tests in the long term. Good idea, we should encourage more tests in a livepatch/ subdir and not clutter up the lib/ directory. >> diff --git a/tools/testing/selftests/livepatch/livepatch-test >> b/tools/testing/selftests/livepatch/livepatch-test >> new file mode 100755 >> index ..798317bf69f6 >> --- /dev/null >> +++ b/tools/testing/selftests/livepatch/livepatch-test > > s/livepatch-test/livepatch-test.sh/ ? > > It seems to be common to add .sh suffix for bash script names. I'll rename with the suffix. >> @@ -0,0 +1,658 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (C) 2018 Joe Lawrence >> + >> +MAX_RETRIES=30 >> +RETRY_INTERVAL=2# seconds >> +BETWEEN_TESTS=20# seconds > > These 20 seconds kept me in a tense (waiting for the final result) > for a very long time ;-) Is there any particular reason for such > a long delay? It certainly builds suspense :) > I wonder if we need a delay at all or if let's say 2 seconds might > be enough. I removed the delays completely and the tests ran successfully. What might be better than a between test delay would be some kind of initial-condition verification, ie, make sure that the test starts/ends with none of the livepatch test modules loaded. For the test cases which load multiple livepatches, is there an easy way to determine the patch stack order from userspace? I think that would be helpful when trying to remove all of them. >> +echo -n "TEST1 ... " >> +dmesg -C >> + >> +load_mod $MOD_TARGET >> +load_mod $MOD_LIVEPATCH >> +wait_for_transition $MOD_LIVEPATCH >> +disable_lp $MOD_LIVEPATCH >> +unload_mod $MOD_LIVEPATCH >> +unload_mod $MOD_TARGET >> + >> +check_result "% modprobe $MOD_TARGET >> +$MOD_TARGET: livepatch_callbacks_mod_init >> +% modprobe $MOD_LIVEPATCH >> +livepatch: enabling patch '$MOD_LIVEPATCH' >> +livepatch: '$MOD_LIVEPATCH': initializing patching transition >> +$MOD_LIVEPATCH: pre_patch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] >> Normal state >> +$MOD_LIVEPATCH: pre_patch_callback: vmlinux >> +livepatch: '$MOD_LIVEPATCH': starting patching transition >> +livepatch: '$MOD_LIVEPATCH': completing patching transition >> +$MOD_LIVEPATCH: post_patch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] >> Normal state >> +$MOD_LIVEPATCH: post_patch_callback: vmlinux >> +livepatch: '$MOD_LIVEPATCH': patching complete >> +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled >> +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition >> +$MOD_LIVEPATCH: pre_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] >> Normal state >> +$MOD_LIVEPATCH: pre_unpatch_callback: vmlinux >> +livepatch: '$MOD_LIVEPATCH': starting unpatching transition >> +livepatch: '$MOD_LIVEPATCH': completing unpatching transition >> +$MOD_LIVEPATCH: post_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] >> Normal state >> +$MOD_LIVEPATCH: post_unpatch_callback: vmlinux &g
Re: [PATCH] selftests/livepatch: introduce tests
On Fri, Apr 06, 2018 at 09:36:46PM -0500, Josh Poimboeuf wrote: > On Wed, Mar 28, 2018 at 03:49:48PM -0400, Joe Lawrence wrote: > > Add a few livepatch modules and simple target modules that the included > > regression suite can run tests against. > > > > Signed-off-by: Joe Lawrence > > --- > > lib/Kconfig.debug | 12 + > > lib/Makefile | 2 + > > lib/livepatch/Makefile | 18 + > > lib/livepatch/test_klp_atomic_replace.c| 69 +++ > > lib/livepatch/test_klp_callbacks_busy.c| 43 ++ > > lib/livepatch/test_klp_callbacks_demo.c| 132 ++ > > lib/livepatch/test_klp_callbacks_demo2.c | 104 > > lib/livepatch/test_klp_callbacks_mod.c | 24 + > > lib/livepatch/test_klp_livepatch.c | 62 +++ > > tools/testing/selftests/Makefile | 1 + > > tools/testing/selftests/livepatch/Makefile | 8 + > > tools/testing/selftests/livepatch/config | 1 + > > tools/testing/selftests/livepatch/functions.sh | 202 > > .../testing/selftests/livepatch/test-callbacks.sh | 526 > > + > > .../testing/selftests/livepatch/test-livepatch.sh | 177 +++ > > .../selftests/livepatch/test-shadow-vars.sh| 13 + > > 16 files changed, 1394 insertions(+) > > create mode 100644 lib/livepatch/Makefile > > create mode 100644 lib/livepatch/test_klp_atomic_replace.c > > create mode 100644 lib/livepatch/test_klp_callbacks_busy.c > > create mode 100644 lib/livepatch/test_klp_callbacks_demo.c > > create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c > > create mode 100644 lib/livepatch/test_klp_callbacks_mod.c > > create mode 100644 lib/livepatch/test_klp_livepatch.c > > create mode 100644 tools/testing/selftests/livepatch/Makefile > > create mode 100644 tools/testing/selftests/livepatch/config > > create mode 100644 tools/testing/selftests/livepatch/functions.sh > > create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh > > create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh > > create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh > > I love this. Nice work! > > As you and Petr discussed, it would be nice to get rid of some of the > delays, and also the callback tests will be very important. I've got v2 WIP that minimizes the delays, cleans up build flags, and adds a basic shadow variable test. Since these tests are based on top of Petr's current patchsets for atomic replace and shadow variables, it probably makes sense for those to merge first. I can post test results to his patchsets if that helps. These tests are basically a mash up of some of the tedious callback Documentation and shadow variable sample livepatches. Since there will be a lot of duplication, should we just remove redundant doc/samples in favor of these tests? -- Joe
[PATCH v3] selftests/livepatch: introduce tests
Add a few livepatch modules and simple target modules that the included regression suite can run tests against. Signed-off-by: Joe Lawrence --- Documentation/livepatch/callbacks.txt | 487 - lib/Kconfig.debug | 12 + lib/Makefile | 2 + lib/livepatch/Makefile | 15 + lib/livepatch/test_klp_atomic_replace.c| 69 +++ lib/livepatch/test_klp_callbacks_busy.c| 43 ++ lib/livepatch/test_klp_callbacks_demo.c| 132 + lib/livepatch/test_klp_callbacks_demo2.c | 104 lib/livepatch/test_klp_callbacks_mod.c | 24 + lib/livepatch/test_klp_livepatch.c | 62 +++ lib/livepatch/test_klp_shadow_vars.c | 236 tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile | 8 + tools/testing/selftests/livepatch/README | 43 ++ tools/testing/selftests/livepatch/config | 1 + tools/testing/selftests/livepatch/functions.sh | 196 +++ .../testing/selftests/livepatch/test-callbacks.sh | 607 + .../testing/selftests/livepatch/test-livepatch.sh | 173 ++ .../selftests/livepatch/test-shadow-vars.sh| 60 ++ 19 files changed, 1788 insertions(+), 487 deletions(-) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 lib/livepatch/test_klp_shadow_vars.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/README create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt index c9776f48e458..6ca2801a6bb9 100644 --- a/Documentation/livepatch/callbacks.txt +++ b/Documentation/livepatch/callbacks.txt @@ -116,490 +116,3 @@ virtnet_probe() initialized its driver's net_device features. A pre/post-patch callback could iterate over all such devices, making a similar change to their hw_features value. (Client functions of the value may need to be updated accordingly.) - - -Test cases -== - -What follows is not an exhaustive test suite of every possible livepatch -pre/post-(un)patch combination, but a selection that demonstrates a few -important concepts. Each test case uses the kernel modules located in -the samples/livepatch/ and assumes that no livepatches are loaded at the -beginning of the test. - - -Test 1 --- - -Test a combination of loading a kernel module and a livepatch that -patches a function in the first module. (Un)load the target module -before the livepatch module: - -- load target module -- load livepatch -- disable livepatch -- unload target module -- unload livepatch - -First load a target module: - - % insmod samples/livepatch/livepatch-callbacks-mod.ko - [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init - -On livepatch enable, before the livepatch transition starts, pre-patch -callbacks are executed for vmlinux and livepatch_callbacks_mod (those -klp_objects currently loaded). After klp_objects are patched according -to the klp_patch, their post-patch callbacks run and the transition -completes: - - % insmod samples/livepatch/livepatch-callbacks-demo.ko - [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo' - [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing patching transition - [ 36.504238] livepatch_callbacks_demo: pre_patch_callback: vmlinux - [ 36.504721] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 36.505849] livepatch: 'livepatch_callbacks_demo': starting patching transition - [ 37.727133] livepatch: 'livepatch_callbacks_demo': completing patching transition - [ 37.727232] livepatch_callbacks_demo: post_patch_callback: vmlinux - [ 37.727860] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 37.728792] livepatch: 'livepatch_callbacks_demo': patching complete - -Similarly, on livepatch disable, pre-patch callbacks run before the -unpatching transition starts. klp_objects are reverte
[PATCH v3] Add livepatch kselftests
Tests run on top of Petr's v11 atomic replace feature and v2 of the shadow variable enhancement patchsets: [PATCH 0/8] livepatch: Atomic replace feature https://lkml.kernel.org/r/20180323120028.31451-1-pmla...@suse.com [PATCH v2 0/2] livepatch: Allocate and free shadow variables more safely https://lkml.kernel.org/r/20180405122315.29065-1-pmla...@suse.com which have been combined into a git tree, then branched: https://github.com/joe-lawrence/linux/tree/klp_kselftest_base https://github.com/joe-lawrence/linux/tree/klp_kselftest_v3 so that the kbuild test robot could verify: Subject: [joe-lawrence:klp_kselftest_v3] BUILD SUCCESS 73f12d67f681e3517a8cdc12ceec07b05543d269 From: kbuild test robot To: Joe Lawrence tree/branch: https://github.com/joe-lawrence/linux klp_kselftest_v3 branch HEAD: 73f12d67f681e3517a8cdc12ceec07b05543d269 selftests/livepatch: introduce tests elapsed time: 95m configs tested: 202 [ ... snip ... ] If anyone knows how to indicate an external git tree base to the bot in the commit message or header letter, let me know. Otherwise I'll have to keep pushing up to github and ignoring its misfires as reported to the list :( changes from v2: - fix module_exit(test_klp_shadow_vars_exit) in test_klp_shadow_vars.c - silence kbuild test robot's "XXX can be static" and "Using plain integer as NULL pointer" complaints - re-run tests with CONFIG_LOCKDEP=y and CONFIG_PROVE_LOCKING=y - use GFP_ATOMIC in test_klp_shadow_vars.c constructor code changes from v1: - Only add $(CC_FLAGS_FTRACE) for target modules - Remove between test delay - Reduce RETRY_INTERVAL to .1 sec - Reduce test_callback_mod's busymod_work_func delay from 60 to 10 sec - s/PASS/ok and s/FAIL/not ok for test output - Move test descriptions from Documentation/livepatch/callbacks.txt into tools/testing/selftests/livepatch/test-callbacks.sh - Add a shadow variable test script and module - Add a short tools/testing/selftests/livepatch/README - to += linux-kselft...@vger.kernel.org - cc += Libor, Nicolai, Artem change from rfc: - SPDX-License-Identifiers - Moved livepatch test modules into lib/livepatch - Renamed livepatch.sh (filename suffix) - Reduced between-test delay time - Split off common functions.sh file - Split into separate livepatch, callbacks, and shadow-vars scrips - Gave the tests short descriptions instead of TEST1, TEST2, etc. Joe Lawrence (1): selftests/livepatch: introduce tests Documentation/livepatch/callbacks.txt | 487 - lib/Kconfig.debug | 12 + lib/Makefile | 2 + lib/livepatch/Makefile | 15 + lib/livepatch/test_klp_atomic_replace.c| 69 +++ lib/livepatch/test_klp_callbacks_busy.c| 43 ++ lib/livepatch/test_klp_callbacks_demo.c| 132 + lib/livepatch/test_klp_callbacks_demo2.c | 104 lib/livepatch/test_klp_callbacks_mod.c | 24 + lib/livepatch/test_klp_livepatch.c | 62 +++ lib/livepatch/test_klp_shadow_vars.c | 236 tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile | 8 + tools/testing/selftests/livepatch/README | 43 ++ tools/testing/selftests/livepatch/config | 1 + tools/testing/selftests/livepatch/functions.sh | 196 +++ .../testing/selftests/livepatch/test-callbacks.sh | 607 + .../testing/selftests/livepatch/test-livepatch.sh | 173 ++ .../selftests/livepatch/test-shadow-vars.sh| 60 ++ 19 files changed, 1788 insertions(+), 487 deletions(-) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 lib/livepatch/test_klp_shadow_vars.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/README create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh -- 1.8.3.1
[PATCH 2/2] sched/debug: adjust newlines for better alignment
Scheduler debug stats include newlines that display out of alignment
when prefixed by timestamps. For example, the dmesg utility:
% echo t > /proc/sysrq-trigger
% dmesg
...
[ 83.124251]
runnable tasks:
S task PID tree-key switches prio wait-time
sum-execsum-sleep
---
At the same time, some syslog utilities (like rsyslog by default) don't
like the additional newlines control characters, saving lines like this
to /var/log/messages:
Mar 16 16:02:29 localhost kernel: #012runnable tasks:#012 S task
PID tree-key ...
Clean these up by moving newline characters to their own SEQ_printf
invocation. This leaves the /proc/sched_debug unchanged, but brings the
entire output into alignment when prefixed:
% echo t > /proc/sysrq-trigger
% dmesg
...
[ 62.410368] runnable tasks:
[ 62.410368] S task PID tree-key switches prio
wait-time sum-execsum-sleep
[ 62.410369]
---
[ 62.410369] I kworker/u12:0 5 1932.215593 332 120
0.00 3.621252 0.00 0 0 /
and no escaped control characters from rsyslog in /var/log/messages:
Mar 16 16:15:06 localhost kernel: runnable tasks:
Mar 16 16:15:06 localhost kernel: S task PID tree-key ...
Signed-off-by: Joe Lawrence
---
kernel/sched/debug.c | 27 ---
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 50026aa2d81e..72c401b3b15c 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -501,12 +501,12 @@ static void print_rq(struct seq_file *m, struct rq *rq,
int rq_cpu)
{
struct task_struct *g, *p;
- SEQ_printf(m,
- "\nrunnable tasks:\n"
- " S task PID tree-key switches prio"
- " wait-time sum-execsum-sleep\n"
- "---"
- "\n");
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "runnable tasks:\n");
+ SEQ_printf(m, " S task PID tree-key switches prio"
+ " wait-time sum-execsum-sleep\n");
+ SEQ_printf(m, "---"
+ "\n");
rcu_read_lock();
for_each_process_thread(g, p) {
@@ -527,9 +527,11 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct
cfs_rq *cfs_rq)
unsigned long flags;
#ifdef CONFIG_FAIR_GROUP_SCHED
- SEQ_printf(m, "\ncfs_rq[%d]:%s\n", cpu, task_group_path(cfs_rq->tg));
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "cfs_rq[%d]:%s\n", cpu, task_group_path(cfs_rq->tg));
#else
- SEQ_printf(m, "\ncfs_rq[%d]:\n", cpu);
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "cfs_rq[%d]:\n", cpu);
#endif
SEQ_printf(m, " .%-30s: %Ld.%06ld\n", "exec_clock",
SPLIT_NS(cfs_rq->exec_clock));
@@ -595,9 +597,11 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct
cfs_rq *cfs_rq)
void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)
{
#ifdef CONFIG_RT_GROUP_SCHED
- SEQ_printf(m, "\nrt_rq[%d]:%s\n", cpu, task_group_path(rt_rq->tg));
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "rt_rq[%d]:%s\n", cpu, task_group_path(rt_rq->tg));
#else
- SEQ_printf(m, "\nrt_rq[%d]:\n", cpu);
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "rt_rq[%d]:\n", cpu);
#endif
#define P(x) \
@@ -624,7 +628,8 @@ void print_dl_rq(struct seq_file *m, int cpu, struct dl_rq
*dl_rq)
{
struct dl_bw *dl_bw;
- SEQ_printf(m, "\ndl_rq[%d]:\n", cpu);
+ SEQ_printf(m, "\n");
+ SEQ_printf(m, "dl_rq[%d]:\n", cpu);
#define PU(x) \
SEQ_printf(m, " .%-30s: %lu\n", #x, (unsigned long)(dl_rq->x))
--
1.8.3.1
[PATCH 0/2] small scheduler debug stat fixups
A customer noticed some misalignment issues in the scheduler debug stats caused by line break/continuations. While the kernel isn't responsible for log rendering, these trivial changes can help clean up the stats display with default settings for dmesg/rsyslog/etc. Note that /proc/sched_debug didn't suffer from these issues and its output remains unchanged by this patchset. Joe Lawrence (2): sched/debug: fix per-task line continuation for console sched/debug: adjust newlines for better alignment kernel/sched/debug.c | 29 + 1 file changed, 17 insertions(+), 12 deletions(-) -- 1.8.3.1
[PATCH 1/2] sched/debug: fix per-task line continuation for console
When the SEQ_printf() macro prints to the console, it runs a simple printk() without KERN_CONT "continued" line printing. The result of this is oddly wrapped task info, for example: % echo t > /proc/sysrq-trigger % dmesg ... runnable tasks: ... [ 29.608611] I [ 29.608613] rcu_sched 8 3252.013846 4087 120 [ 29.608614] 0.0029.090111 0.00 [ 29.608615] 0 0 [ 29.608616] / Modify SEQ_printf to use pr_cont() for expected one-line results: % echo t > /proc/sysrq-trigger % dmesg ... runnable tasks: ... [ 106.716329] Scpuhp/537 2006.31502614 120 0.00 0.496893 0.00 0 0 / Signed-off-by: Joe Lawrence --- kernel/sched/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c index 1ca0130ed4f9..50026aa2d81e 100644 --- a/kernel/sched/debug.c +++ b/kernel/sched/debug.c @@ -32,7 +32,7 @@ if (m) \ seq_printf(m, x); \ else\ - printk(x); \ + pr_cont(x); \ } while (0) /* -- 1.8.3.1
Re: [PATCH 1/2] sched/debug: fix per-task line continuation for console
On 03/19/2018 04:17 PM, Peter Zijlstra wrote:
> On Mon, Mar 19, 2018 at 02:35:54PM -0400, Joe Lawrence wrote:
>> When the SEQ_printf() macro prints to the console, it runs a simple
>> printk() without KERN_CONT "continued" line printing. The result of
>> this is oddly wrapped task info, for example:
>>
>> % echo t > /proc/sysrq-trigger
>> % dmesg
>> ...
>> runnable tasks:
>> ...
>> [ 29.608611] I
>> [ 29.608613] rcu_sched 8 3252.013846 4087 120
>> [ 29.608614] 0.0029.090111 0.00
>> [ 29.608615] 0 0
>> [ 29.608616] /
>>
>> Modify SEQ_printf to use pr_cont() for expected one-line results:
>>
>> % echo t > /proc/sysrq-trigger
>> % dmesg
>> ...
>> runnable tasks:
>> ...
>> [ 106.716329] Scpuhp/537 2006.31502614 120
>> 0.00 0.496893 0.00 0 0 /
>>
>> Signed-off-by: Joe Lawrence
>> ---
>> kernel/sched/debug.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
>> index 1ca0130ed4f9..50026aa2d81e 100644
>> --- a/kernel/sched/debug.c
>> +++ b/kernel/sched/debug.c
>> @@ -32,7 +32,7 @@
>> if (m) \
>> seq_printf(m, x); \
>> else\
>> -printk(x); \
>> +pr_cont(x); \
>
> That used to work I think.. I think someone changed how printk() behaves
> somewhere along the lines.
>
Hi Peter,
This code:
printk("printk one");
printk("printk two\n");
pr_cont("\n");
pr_cont("pr_cont one");
pr_cont("pr_cont two\n");
pr_cont("pr_cont first line\n");
pr_cont("\n");
pr_cont("\n");
pr_cont("pr_cont next line");
Creates this output:
% uname -r
4.16.0-rc5+
% dmesg
[ 575.221280] printk one
[ 575.221281] printk two
[ 575.221282] pr_cont onepr_cont two
[ 575.221283] pr_cont first line
I don't have the commit offhand that changed the printk behavior, but
from observation:
1 - printk implies a trailing newline:
https://lwn.net/Articles/732420/
2 - pr_cont seems to eat redundant newlines
> Does pr_cont("\n") DTRT? it seems like something weird.
>
Yeah, pr_cont() is kind of a hack. It will terminate a line if that's
the first newline, but as demonstrated above, I don't think it likes
extra newline chars.
A better fix would be to marshal the text into temp buffer then dump it
out. Dunno if you prefer that kind of churn for these stats.
-- Joe
[PATCH v2] Add livepatch kselftests
Round two cleans up a few misc script and build items, adds a shadow variable test, and reduces the total livepatch kselftest runtime to ~45 seconds. The tests run on top of Petr's v11 atomic replace feature and v2 of the shadow variable enhancement patchsets: [PATCH 0/8] livepatch: Atomic replace feature https://lkml.kernel.org/r/20180323120028.31451-1-pmla...@suse.com [PATCH v2 0/2] livepatch: Allocate and free shadow variables more safely https://lkml.kernel.org/r/20180405122315.29065-1-pmla...@suse.com Questions for v3: - Should we split off the atomic replace and shadow variable update tests so that the this patchset could be merged before the ones listed above? - I didn't remove any of the sample modules. If anyone thinks any of them should go, let me know. They serve as nice, simple examples so I thought they should all stay. - Module naming convention: to make the test script easier to grep module names and filenames, I broke with livepatch convention and used underscores instead of dashes. I didn't think it worth the regex foo to flip back and forth in the test script. - More tests from Libor and Nicolai would be welcome! -- Maybe we separate quicktests (like these) from longer tests if needed? Here's a sample output from a successful test run: % time make -C tools/testing/selftests TARGETS=livepatch run_tests make: Entering directory `/root/linux/tools/testing/selftests' make[1]: Entering directory `/root/linux/tools/testing/selftests/livepatch' make[1]: Nothing to be done for `all'. make[1]: Leaving directory `/root/linux/tools/testing/selftests/livepatch' make[1]: Entering directory `/root/linux/tools/testing/selftests/livepatch' TAP version 13 selftests: test-livepatch.sh TEST: basic function patching ... ok TEST: multiple livepatches ... ok TEST: atomic replace livepatch ... ok ok 1..1 selftests: test-livepatch.sh [PASS] selftests: test-callbacks.sh TEST: target module before livepatch ... ok TEST: module_coming notifier ... ok TEST: module_going notifier ... ok TEST: module_coming and module_going notifiers ... ok TEST: target module not present ... ok TEST: pre-patch callback -ENODEV ... ok TEST: module_coming + pre-patch callback -ENODEV ... ok TEST: multiple target modules ... ok TEST: busy target module ... ok TEST: multiple livepatches ... ok TEST: atomic replace ... ok ok 1..2 selftests: test-callbacks.sh [PASS] selftests: test-shadow-vars.sh TEST: basic shadow variable API ... ok ok 1..3 selftests: test-shadow-vars.sh [PASS] make[1]: Leaving directory `/root/linux/tools/testing/selftests/livepatch' make: Leaving directory `/root/linux/tools/testing/selftests' real0m46.166s user0m0.436s sys 0m1.244s changes from v1: - Only add $(CC_FLAGS_FTRACE) for target modules - Remove between test delay - Reduce RETRY_INTERVAL to .1 sec - Reduce test_callback_mod's busymod_work_func delay from 60 to 10 sec - s/PASS/ok and s/FAIL/not ok for test output - Move test descriptions from Documentation/livepatch/callbacks.txt into tools/testing/selftests/livepatch/test-callbacks.sh - Add a shadow variable test script and module - Add a short tools/testing/selftests/livepatch/README - to += linux-kselft...@vger.kernel.org - cc += Libor, Nicolai, Artem change from rfc: - SPDX-License-Identifiers - Moved livepatch test modules into lib/livepatch - Renamed livepatch.sh (filename suffix) - Reduced between-test delay time - Split off common functions.sh file - Split into separate livepatch, callbacks, and shadow-vars scrips - Gave the tests short descriptions instead of TEST1, TEST2, etc. Joe Lawrence (1): selftests/livepatch: introduce tests Documentation/livepatch/callbacks.txt | 487 - lib/Kconfig.debug | 12 + lib/Makefile | 2 + lib/livepatch/Makefile | 15 + lib/livepatch/test_klp_atomic_replace.c| 69 +++ lib/livepatch/test_klp_callbacks_busy.c| 43 ++ lib/livepatch/test_klp_callbacks_demo.c| 132 + lib/livepatch/test_klp_callbacks_demo2.c | 104 lib/livepatch/test_klp_callbacks_mod.c | 24 + lib/livepatch/test_klp_livepatch.c | 62 +++ lib/livepatch/test_klp_shadow_vars.c | 235 tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile | 8 + tools/testing/selftests/livepatch/config | 1 + tools/testing/selftests/livepatch/functions.sh | 196 +++ .../testing/selftests/livepatch/test-callbacks.sh | 607 + .../testing/selfte
[PATCH v2] selftests/livepatch: introduce tests
Add a few livepatch modules and simple target modules that the included regression suite can run tests against. Signed-off-by: Joe Lawrence --- Documentation/livepatch/callbacks.txt | 487 - lib/Kconfig.debug | 12 + lib/Makefile | 2 + lib/livepatch/Makefile | 15 + lib/livepatch/test_klp_atomic_replace.c| 69 +++ lib/livepatch/test_klp_callbacks_busy.c| 43 ++ lib/livepatch/test_klp_callbacks_demo.c| 132 + lib/livepatch/test_klp_callbacks_demo2.c | 104 lib/livepatch/test_klp_callbacks_mod.c | 24 + lib/livepatch/test_klp_livepatch.c | 62 +++ lib/livepatch/test_klp_shadow_vars.c | 235 tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile | 8 + tools/testing/selftests/livepatch/config | 1 + tools/testing/selftests/livepatch/functions.sh | 196 +++ .../testing/selftests/livepatch/test-callbacks.sh | 607 + .../testing/selftests/livepatch/test-livepatch.sh | 173 ++ .../selftests/livepatch/test-shadow-vars.sh| 60 ++ 18 files changed, 1744 insertions(+), 487 deletions(-) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 lib/livepatch/test_klp_shadow_vars.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt index c9776f48e458..6ca2801a6bb9 100644 --- a/Documentation/livepatch/callbacks.txt +++ b/Documentation/livepatch/callbacks.txt @@ -116,490 +116,3 @@ virtnet_probe() initialized its driver's net_device features. A pre/post-patch callback could iterate over all such devices, making a similar change to their hw_features value. (Client functions of the value may need to be updated accordingly.) - - -Test cases -== - -What follows is not an exhaustive test suite of every possible livepatch -pre/post-(un)patch combination, but a selection that demonstrates a few -important concepts. Each test case uses the kernel modules located in -the samples/livepatch/ and assumes that no livepatches are loaded at the -beginning of the test. - - -Test 1 --- - -Test a combination of loading a kernel module and a livepatch that -patches a function in the first module. (Un)load the target module -before the livepatch module: - -- load target module -- load livepatch -- disable livepatch -- unload target module -- unload livepatch - -First load a target module: - - % insmod samples/livepatch/livepatch-callbacks-mod.ko - [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init - -On livepatch enable, before the livepatch transition starts, pre-patch -callbacks are executed for vmlinux and livepatch_callbacks_mod (those -klp_objects currently loaded). After klp_objects are patched according -to the klp_patch, their post-patch callbacks run and the transition -completes: - - % insmod samples/livepatch/livepatch-callbacks-demo.ko - [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo' - [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing patching transition - [ 36.504238] livepatch_callbacks_demo: pre_patch_callback: vmlinux - [ 36.504721] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 36.505849] livepatch: 'livepatch_callbacks_demo': starting patching transition - [ 37.727133] livepatch: 'livepatch_callbacks_demo': completing patching transition - [ 37.727232] livepatch_callbacks_demo: post_patch_callback: vmlinux - [ 37.727860] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 37.728792] livepatch: 'livepatch_callbacks_demo': patching complete - -Similarly, on livepatch disable, pre-patch callbacks run before the -unpatching transition starts. klp_objects are reverted, post-patch -callbacks execute and the transition completes: - - % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_dem
Re: [PATCH v2] selftests/livepatch: introduce tests
On 04/10/2018 04:00 PM, Josh Poimboeuf wrote:
> On Tue, Apr 10, 2018 at 11:15:54AM -0400, Joe Lawrence wrote:
>> +static void test_klp_shadow_vars_exit(void)
>> +{
>> +}
>> +
>> +module_init(test_klp_shadow_vars_init);
>> +module_init(test_klp_shadow_vars_exit);
>
> For this last line, s/module_init/module_exit/, though I think the exit
> function can just be removed altogether?
D'oh workspace / git user error, I posted an older version :(
But the exit function seems to be required if an init function is
provided. Here I omitted the exit function:
% modprobe test_klp_shadow_vars
% lsmod | grep test_klp_shadow_vars
test_klp_shadow_vars16384 0
% rmmod test_klp_shadow_vars
rmmod: ERROR: could not remove 'test_klp_shadow_vars': Device or
resource busy
rmmod: ERROR: could not remove module test_klp_shadow_vars: Device or
resource busy
and from kernel/module.c
SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
unsigned int, flags)
...
/* If it has an init func, it must have an exit func to unload*/
if (mod->init && !mod->exit) {
forced = try_force_unload(flags);
if (!forced) {
/* This module can't be removed */
ret = -EBUSY;
goto out;
}
}
...
>
> Also I get the following bug (run on latest Linus master + Petr's two
> patch sets):
>
> [ 106.302072] % modprobe test_klp_shadow_vars
> [ 106.311165] test_klp_shadow_vars: klp_shadow_get(obj=PTR5, id=0x1234) =
> PTR0
> [ 106.313080] test_klp_shadow_vars: got expected NULL result
> [ 106.314811] BUG: sleeping function called from invalid context at
> mm/slab.h:421
> [ 106.316518] in_atomic(): 1, irqs_disabled(): 1, pid: 2254, name: modprobe
> [ 106.318107] 1 lock held by modprobe/2254:
> [ 106.319332] #0: d0851080 (klp_shadow_lock){}, at:
> __klp_shadow_get_or_alloc+0x88/0x1b0
> [ 106.321220] irq event stamp: 4408
> [ 106.322176] hardirqs last enabled at (4407): []
> console_unlock+0x44e/0x680
> [ 106.323598] hardirqs last disabled at (4408): []
> _raw_spin_lock_irqsave+0x27/0x90
> [ 106.325041] softirqs last enabled at (4404): []
> __do_softirq+0x39b/0x4fc
> [ 106.326469] softirqs last disabled at (4367): []
> irq_exit+0xe0/0xf0
> [ 106.327901] Preemption disabled at:
> [ 106.327905] [] __klp_shadow_get_or_alloc+0x88/0x1b0
> [ 106.330117] CPU: 7 PID: 2254 Comm: modprobe Tainted: G K
> 4.16.0+ #60
> [ 106.331565] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.10.2-2.fc27 04/01/2014
> [ 106.333143] Call Trace:
> [ 106.334011] dump_stack+0x8e/0xd5
> [ 106.334962] ___might_sleep+0x185/0x260
> [ 106.335997] ? shadow_dtor+0x40/0x40 [test_klp_shadow_vars]
> [ 106.337170] __might_sleep+0x4a/0x80
> [ 106.338137] kmem_cache_alloc_trace+0x20b/0x300
> [ 106.339183] ? ptr_id+0x5c/0xd0 [test_klp_shadow_vars]
> [ 106.340341] ? shadow_dtor+0x40/0x40 [test_klp_shadow_vars]
> [ 106.341506] ptr_id+0x5c/0xd0 [test_klp_shadow_vars]
> [ 106.342725] shadow_ctor+0x20/0x40 [test_klp_shadow_vars]
> [ 106.343998] __klp_shadow_get_or_alloc+0xc4/0x1b0
> [ 106.345184] ? shadow_dtor+0x40/0x40 [test_klp_shadow_vars]
> [ 106.346494] klp_shadow_alloc+0x10/0x20
> [ 106.347583] shadow_alloc+0x28/0xa0 [test_klp_shadow_vars]
> [ 106.348871] ? shadow_free_all+0x40/0x40 [test_klp_shadow_vars]
> [ 106.350200] test_klp_shadow_vars_init+0x96/0x400 [test_klp_shadow_vars]
> [ 106.351646] ? shadow_free_all+0x40/0x40 [test_klp_shadow_vars]
> [ 106.352949] do_one_initcall+0x61/0x37f
> [ 106.353963] ? rcu_read_lock_sched_held+0x79/0x80
> [ 106.355040] ? kmem_cache_alloc_trace+0x29d/0x300
> [ 106.356098] ? do_init_module+0x27/0x213
> [ 106.357090] do_init_module+0x5f/0x213
> [ 106.358047] load_module+0x2815/0x2e70
> [ 106.358992] ? vfs_read+0x12d/0x150
> [ 106.359920] SYSC_finit_module+0xfc/0x120
> [ 106.360870] ? SYSC_finit_module+0xfc/0x120
> [ 106.361839] SyS_finit_module+0xe/0x10
> [ 106.362753] do_syscall_64+0x7e/0x240
> [ 106.363645] entry_SYSCALL_64_after_hwframe+0x42/0xb7
> [ 106.364711] RIP: 0033:0x7f26d0d40b19
> [ 106.365799] RSP: 002b:7ffee1de19a8 EFLAGS: 0246 ORIG_RAX:
> 0139
> [ 106.367306] RAX: ffda RBX: 5636550d54b0 RCX:
> 7f26d0d40b19
> [ 106.368573] RDX: RSI: 563654134186 RDI:
> 0003
> [ 106.369950] RBP: 563654134186 R08: R09:
> 5636550d4270
> [ 106.371164] R10: 0003 R11: 0246 R12:
>
> [ 106.372422] R13: 5636550d53b0 R14:
Re: [PATCH v10 00/10] livepatch: Atomic replace feature
On 03/08/2018 10:01 AM, Petr Mladek wrote: > On Wed 2018-03-07 16:55:53, Joe Lawrence wrote: >> Running against v10, callbacks seem to be good up until I disable an >> atomic replace patch. My understanding is that the original patch's >> unpatch callbacks should be skipped (as they were). I was surprised to >> see that atomic replacement patch only ran it's post-unpatch callback. > > Great catch! > > I guess that it is caused by the heuristic used in > klp_unpatch_object() to decide whether the object is patched > or not. > > We need to change the state only when manipulating the > statically defined functions. > > Thanks a lot for so extensive testing!!! Sorry for not getting to this series sooner. I'm trying to refit these tests into a kselftest so we can more easily reuse them. Still hacking away at it, but I'll post when something soon to start a livepatch selftests conversation :) -- Joe
Re: [RFC PATCH 0/1] support ftrace and -ffunction-sections
On 11/20/2018 03:19 PM, Joe Lawrence wrote:
> Hi Steve,
>
> I noticed that ftrace does not currently support functions built with
> the -ffunction-sections option as they end up in .text.
> ELF sections, never making into the __mcount_loc section.
>
> I modified the recordmcount scripts to handle such .text. section
> prefixes and this appears to work on x86_64, ppc64le, and s390x -- at
> least for simple modules, including the "Test trace_printk from module"
> ftrace self-test.
>
> That said, I did notice 90ad4052e85c ("kbuild: avoid conflict between
> -ffunction-sections and -pg on gcc-4.7") which indicates that the kernel
> still supports versions of gcc which may not play well with ftrace and
> -ffunction-sections.
>
> With that limitation in mind, can we support ftracing of functions in
> such sections for compiler versions that do support it? (fwiw, gcc
> v4.8.5 seems happy) And then if so, what additional testing or coding
> would need to be done to be confident that it is safe? Is matching on
> ".text.*" too inclusive?
>
Gentle ping... I took a dive through the rhkl-archives and found a few
older discussions:
[PATCH] scripts/recordmcount.pl: Support build with -ffunction-sections.
https://lore.kernel.org/lkml/CAFbHwiRtBaHkpZqTm6VZ=fCJcyu+dsdpo_kxMHy1egce=rt...@mail.gmail.com/
and related LWN article:
The source of the e1000e corruption bug
https://lwn.net/Articles/304105/
Catching up with those, I assume that this has never been implemented in
the past due to fear of ftrace modifying a potentially freed section
(and bricking NICs in the process :(
Looking through the kernel sources (like Will in 2008) I don't see any
code jumping out at me that frees code other than .init. However a
quick code inspection is no guarantee.
Assuming the same use-after-free reservation holds true today:
1: Is there any reasonable way to mark code sections (pages?) as
in-use to avoid memory freeing mechanisms from releasing them? The
logic for .init is mostly arch-specific, so there could be many
different ways random arches may try to pull this off.
2: Would/could it be safer to restrict __mcount_loc detection of
".text.*" sections to modules? The recordmcount.pl script already
knows about is_module... that information could be provided to
recordmcount.c as well for consideration.
-- Joe
[RFC PATCH 0/1] support ftrace and -ffunction-sections
Hi Steve,
I noticed that ftrace does not currently support functions built with
the -ffunction-sections option as they end up in .text.
ELF sections, never making into the __mcount_loc section.
I modified the recordmcount scripts to handle such .text. section
prefixes and this appears to work on x86_64, ppc64le, and s390x -- at
least for simple modules, including the "Test trace_printk from module"
ftrace self-test.
That said, I did notice 90ad4052e85c ("kbuild: avoid conflict between
-ffunction-sections and -pg on gcc-4.7") which indicates that the kernel
still supports versions of gcc which may not play well with ftrace and
-ffunction-sections.
With that limitation in mind, can we support ftracing of functions in
such sections for compiler versions that do support it? (fwiw, gcc
v4.8.5 seems happy) And then if so, what additional testing or coding
would need to be done to be confident that it is safe? Is matching on
".text.*" too inclusive?
Thanks,
-- Joe
Joe Lawrence (1):
scripts/recordmcount.{c,pl}: support -ffunction-sections .text.*
section names
scripts/recordmcount.c | 2 +-
scripts/recordmcount.pl | 13 +
2 files changed, 14 insertions(+), 1 deletion(-)
--
1.8.3.1
[RFC PATCH 1/1] scripts/recordmcount.{c,pl}: support -ffunction-sections .text.* section names
When building with -ffunction-sections, the compiler will place each
function into its own ELF section, prefixed with ".text". For example,
a simple test module with functions test_module_do_work() and
test_module_wq_func():
% objdump --section-headers test_module.o | awk '/\.text/{print $2}'
.text
.text.test_module_do_work
.text.test_module_wq_func
.init.text
.exit.text
Adjust the recordmcount scripts to look for ".text" as a section name
prefix. This will ensure that those functions will be included in the
__mcount_loc relocations:
% objdump --reloc --section __mcount_loc test_module.o
OFFSET TYPE VALUE
R_X86_64_64 .text.test_module_do_work
0008 R_X86_64_64 .text.test_module_wq_func
0010 R_X86_64_64 .init.text
Signed-off-by: Joe Lawrence
---
scripts/recordmcount.c | 2 +-
scripts/recordmcount.pl | 13 +
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 895c40e8679f..a50a2aa963ad 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -397,7 +397,7 @@ static uint32_t w2nat(uint16_t const x)
static int
is_mcounted_section_name(char const *const txtname)
{
- return strcmp(".text", txtname) == 0 ||
+ return strncmp(".text", txtname, 5) == 0 ||
strcmp(".init.text", txtname) == 0 ||
strcmp(".ref.text", txtname) == 0 ||
strcmp(".sched.text",txtname) == 0 ||
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index f599031260d5..68841d01162c 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -142,6 +142,11 @@ my %text_sections = (
".text.unlikely" => 1,
);
+# Acceptable section-prefixes to record.
+my %text_section_prefixes = (
+ ".text." => 1,
+);
+
# Note: we are nice to C-programmers here, thus we skip the '||='-idiom.
$objdump = 'objdump' if (!$objdump);
$objcopy = 'objcopy' if (!$objcopy);
@@ -519,6 +524,14 @@ while () {
# Only record text sections that we know are safe
$read_function = defined($text_sections{$1});
+ if (!$read_function) {
+ foreach my $prefix (keys %text_section_prefixes) {
+ if (substr($1, 0, length $prefix) eq $prefix) {
+ $read_function = 1;
+ last;
+ }
+ }
+ }
# print out any recorded offsets
update_funcs();
--
1.8.3.1
Re: [RFC PATCH 0/1] support ftrace and -ffunction-sections
On 11/27/2018 09:09 PM, Steven Rostedt wrote: > On Tue, 27 Nov 2018 15:27:14 -0500 > Joe Lawrence wrote: > >> Gentle ping... I took a dive through the rhkl-archives and found a few >> older discussions: > > Thanks for the reminder, my INBOX is totally out of control with > Plumbers followed by Turkey Day. > > [ ... snip ... ] > > I'm fine with just applying your patch. Today, for x86, there's a gcc > option that adds the __mcount_loc automatically without doing any > whitelisting (it doesn't run recordmcount.*). It just adds anything that > is traced, thus it has to work for all possible cases now. > Ah right, -mcount-record ... I must have been using an older gcc w/o -mcount-record in my testing. Thanks for taking a look and merging. Regards, -- Joe
Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption
On Wed, Nov 21, 2018 at 07:28:01PM -0500, Steven Rostedt wrote:
>
> [ ... snip ... ]
>
> Feel free to test this! I'll be pushing this to linux-next and let it
> sit there a week or so before pushing it to Linus.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> ftrace/urgent
Hi Steve,
With your ftrace/urgent branch linked above, if I try a quick
function_graph test like the following:
SYSFS=/sys/kernel/debug/tracing
echo 0 > "$SYSFS/tracing_on"
echo cmdline_proc_show > "$SYSFS/set_graph_function"
echo function_graph > "$SYSFS/current_tracer"
echo 1 > "$SYSFS/tracing_on"
I see a bunch of scheduler interrupt functions in the trace/trace_pipe
without even invoking cmdline_proc_show().
This tests works as expected with Linux 4.20-rc3 though:
% cat /sys/kernel/debug/tracing/trace_pipe
2) | cmdline_proc_show() {
2) 0.320 us|seq_puts();
2) 0.030 us|seq_putc();
2) 1.352 us| }
Operator error, or did the patchset break something?
Regards,
-- Joe
Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption
On 11/28/2018 11:24 PM, Steven Rostedt wrote:
> On Wed, 28 Nov 2018 22:29:36 -0500
> Steven Rostedt wrote:
>
>> Does this patch fix it for you?
>
> Take 2. I realized that the reason for the interrupts being traced is
> because it is more likely to interrupt when the depth is already 0,
> setting it to 1 causing the interrupt to think it's already in a
> function that wants to be traced (when that wasn't decided yet).
>
> The fix uses a bit that gets set when we want to trace (and will trace
> till that bit is cleared). That bit gets cleared on the return
> function when depth is zero. But if we are tracing something in a
> interrupt that happened to interrupt the beginning of a function that
> already set the depth to 0, then we need to clear the bit at depth of 1
> not zero (and 2 if we want to trace a function that interrupted a
> softirq, that interrupted a start of normal function. and 3 if we want
> to trace an NMI function that had the very unlikely case of
> interrupting a start of a interrupt function, that interrupted the
> start of a softirq function, that interrupted a start of a normal
> context function!).
>
> If that happens then we will continue to trace functions when we don't
> want to. To solve that, I need to record the depth (0-3) when the bit
> is set, and clear it when the return function is at that same depth
> (0-3).
>
> -- Steve
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 3b8c0e24ab30..447bd96ee658 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -512,12 +512,44 @@ enum {
> * can only be modified by current, we can reuse trace_recursion.
> */
> TRACE_IRQ_BIT,
> +
> + /* Set if the function is in the set_graph_function file */
> + TRACE_GRAPH_BIT,
> +
> + /*
> + * In the very unlikely case that an interrupt came in
> + * at a start of graph tracing, and we want to trace
> + * the function in that interrupt, the depth can be greater
> + * than zero, because of the preempted start of a previous
> + * trace. In an even more unlikely case, depth could be 2
> + * if a softirq interrupted the start of graph tracing,
> + * followed by an interrupt preempting a start of graph
> + * tracing in the softirq, and depth can even be 3
> + * if an NMI came in at the start of an interrupt function
> + * that preempted a softirq start of a function that
> + * preempted normal context Luckily, it can't be
> + * greater than 3, so the next two bits are a mask
> + * of what the depth is when we set TRACE_GRAPH_BIT
> + */
> +
> + TRACE_GRAPH_DEPTH_START_BIT,
> + TRACE_GRAPH_DEPTH_END_BIT,
> };
>
> #define trace_recursion_set(bit) do { (current)->trace_recursion |=
> (1<<(bit)); } while (0)
> #define trace_recursion_clear(bit) do { (current)->trace_recursion &=
> ~(1<<(bit)); } while (0)
> #define trace_recursion_test(bit)((current)->trace_recursion &
> (1<<(bit)))
>
> +#define trace_recursion_depth() \
> + (((current)->trace_recursion >> TRACE_GRAPH_DEPTH_START_BIT) & 3)
> +#define trace_recursion_set_depth(depth) \
> + do {\
> + current->trace_recursion &= \
> + ~(3 << TRACE_GRAPH_DEPTH_START_BIT);\
> + current->trace_recursion |= \
> + ((depth) & 3) << TRACE_GRAPH_DEPTH_START_BIT; \
> + } while (0)
> +
> #define TRACE_CONTEXT_BITS 4
>
> #define TRACE_FTRACE_START TRACE_FTRACE_BIT
> @@ -843,8 +875,9 @@ extern void __trace_graph_return(struct trace_array *tr,
> extern struct ftrace_hash *ftrace_graph_hash;
> extern struct ftrace_hash *ftrace_graph_notrace_hash;
>
> -static inline int ftrace_graph_addr(unsigned long addr)
> +static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
> {
> + unsigned long addr = trace->func;
> int ret = 0;
>
> preempt_disable_notrace();
> @@ -855,6 +888,14 @@ static inline int ftrace_graph_addr(unsigned long addr)
> }
>
> if (ftrace_lookup_ip(ftrace_graph_hash, addr)) {
> +
> + /*
> + * This needs to be cleared on the return functions
> + * when the depth is zero.
> + */
> + trace_recursion_set(TRACE_GRAPH_BIT);
> + trace_recursion_set_depth(trace->depth);
> +
> /*
>* If no irqs are to be traced, but a set_graph_function
>* is set, and called by an interrupt handler, we still
> @@ -872,6 +913,13 @@ static inline int ftrace_graph_addr(unsigned long addr)
> return ret;
> }
>
> +static inline void ftrace_graph_addr_finish(struct ftrace_graph_ret *trace)
> +{
> + if (trace_recursion_test(TRACE_GRAPH_BIT) &&
> + trace->depth == trace_recursion_depth())
> + trace_recursion_clear(TRACE_GRAPH_BIT)
Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption
On 11/29/2018 11:17 AM, Steven Rostedt wrote: > On Thu, 29 Nov 2018 10:08:55 -0500 > Joe Lawrence wrote: > >> With the "take 2" patch, I only see smp_irq_work_interrupt() graph when >> the graph_function is in progress.. for example: > > In other words it works :-) > > Can I get a Tested-by from you? > Yup, thanks for fixing! Tested-by: Joe Lawrence -- Joe
Re: [PATCH v14 01/11] livepatch: Change unsigned long old_addr -> void *old_func in struct klp_func
On Thu, Nov 29, 2018 at 10:44:21AM +0100, Petr Mladek wrote:
> The address of the to be patched function and new function is stored
> in struct klp_func as:
>
> void *new_func;
> unsigned long old_addr;
>
> The different naming scheme and type is derived from the way how
^^^
s/the way how/the way
> the addresses are set. @old_addr is assigned at runtime using
> kallsyms-based search. @new_func is statically initialized,
> for example:
>
> static struct klp_func funcs[] = {
> {
> .old_name = "cmdline_proc_show",
> .new_func = livepatch_cmdline_proc_show,
> }, { }
> };
>
> This patch changes unsigned log old_addr -> void *old_func. It removes
s/unsigned log/unsigned long
> some confusion when these address are later used in the code. It is
> motivated by a followup patch that adds special NOP struct klp_func
> where we want to assign func->new_func = func->old_addr respectively
> func->new_func = func->old_func.
>
> This patch does not modify the existing behavior.
>
> Suggested-by: Josh Poimboeuf
> Signed-off-by: Petr Mladek
> ---
Acked-by: Joe Lawrence
-- Joe
Re: [PATCH v14 02/11] livepatch: Shuffle klp_enable_patch()/klp_disable_patch() code
On Thu, Nov 29, 2018 at 10:44:22AM +0100, Petr Mladek wrote: > We are going to simplify the API and code by removing the registration > step. This would require calling init/free functions from enable/disable > ones. > > This patch just moves the code to prevent more forward declarations. > > This patch does not change the code except of two forward declarations. ^ s/except of/except for > > Signed-off-by: Petr Mladek > --- Acked-by: Joe Lawrence -- Joe
Re: [PATCH v14 03/11] livepatch: Consolidate klp_free functions
On Thu, Nov 29, 2018 at 10:44:23AM +0100, Petr Mladek wrote:
> The code for freeing livepatch structures is a bit scattered and tricky:
>
> + direct calls to klp_free_*_limited() and kobject_put() are
> used to release partially initialized objects
>
> + klp_free_patch() removes the patch from the public list
> and releases all objects except for patch->kobj
>
> + object_put(&patch->kobj) and the related wait_for_completion()
> are called directly outside klp_mutex; this code is duplicated;
>
> Now, we are going to remove the registration stage to simplify the API
> and the code. This would require handling more situations in
> klp_enable_patch() error paths.
>
> More importantly, we are going to add a feature called atomic replace.
> It will need to dynamically create func and object structures. We will
> want to reuse the existing init() and free() functions. This would
> create even more error path scenarios.
>
> [*] We need our own flag. Note that kobject_put() cannot be called safely
> when kobj.state_initialized is set. This flag is true when kobject_add()
> part failed. And it is never cleared.
> [*] We need our own flag. Note that kobject_put() cannot be called safely
> when kobj.state_initialized is set. This flag is true when kobject_add()
> part failed. And it is never cleared.
> This patch implements a more clever free functions:
^
re-wording suggestions: "simpler", "clearer", "more straightforward"
>
> + checks kobj_alive flag instead of @limit[*]
>
> + initializes patch->list early so that the check for empty list
> always works
>
> + The action(s) that has to be done outside klp_mutex are done
> in separate klp_free_patch_finish() function. It waits only
> when patch->kobj was really released via the _start() part.
>
> The patch does not change the existing behavior.
>
> [*] We need our own flag. Note that kobject_put() cannot be called safely
> when kobj.state_initialized is set. This flag is true when kobject_add()
> part failed. And it is never cleared.
Isn't kobj.state_initialized also true in the ordinary kobject_put() case
where kobject_add() succeeded?
If so, this note could be modified slightly:
(minimal change)
[*] We need our own flag. Note that kobject_put() cannot be called safely
just because kobj.state_initialized is set. This flag is even true when
kobject_add()
part failed. And it is never cleared.
-- or --
(rewording)
[*] We need our own flag to track that the kobject was successfully
added to the hierarchy. Note that kobj.state_initialized only
indicates that kobject has been initialized, not whether is has been
added (and needs to be removed on cleanup).
>
> Signed-off-by: Petr Mladek
> Cc: Josh Poimboeuf
> Cc: Miroslav Benes
> Cc: Jessica Yu
> Cc: Jiri Kosina
> Cc: Jason Baron
> ---
Acked-by: Joe Lawrence
>
> [ ... snip ... ]
>
> +static int klp_init_patch(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> + int ret;
> +
> + mutex_lock(&klp_mutex);
> +
> + ret = klp_init_patch_before_free(patch);
> if (ret) {
> mutex_unlock(&klp_mutex);
> return ret;
> }
>
I believe klp_init_patch_before_free() accumulates more responsibilities
later in the patchset, but I'll ask here: does it really need the
klp_mutex since it looks to be operating only on the klp_patch, its
objects and functions?
-- Joe
Re: [PATCH v14 04/11] livepatch: Refuse to unload only livepatches available during a forced transition
On Thu, Nov 29, 2018 at 10:44:24AM +0100, Petr Mladek wrote: > module_put() is currently never called in klp_complete_transition() when > klp_force is set. As a result, we might keep the reference count even when > klp_enable_patch() fails and klp_cancel_transition() is called. > > This might make an assumption that a module might get blocked in some ^^ re-wording suggestion: "give the impression" > strange init state. Fortunately, it is not the case. The reference count > is ignored when mod->init fails and erroneous modules are always removed. > > Anyway, this might make some confusion. Instead, this patch moves ^^^ re-wording suggestions: "create confusion" or "be confusing" > the global klp_forced flag into struct klp_patch. As a result, > we block only modules that might still be in use after a forced > transition. Newly loaded livepatches might be eventually completely > removed later. > > It is not a big deal. But the code is at least consistent with > the reality. > > Signed-off-by: Petr Mladek Acked-by: Joe Lawrence -- Joe
Re: [PATCH v14 05/11] livepatch: Simplify API by removing registration step
On Thu, Nov 29, 2018 at 10:44:25AM +0100, Petr Mladek wrote: > The possibility to re-enable a registered patch was useful for immediate > patches where the livepatch module had to stay until the system reboot. > The improved consistency model allows to achieve the same result by > unloading and loading the livepatch module again. > > Also we are going to add a feature called atomic replace. It will allow > to create a patch that would replace all already registered patches. > The aim is to handle dependent patches more securely. It will obsolete > the stack of patches that helped to handle the dependencies so far. > Then it might be unclear when a cumulative patch re-enabling is safe. > > It would be complicated to support the many modes. Instead we could > actually make the API and code easier to understand. > > This patch removes the two step public API. All the checks and init calls > are moved from klp_register_patch() to klp_enabled_patch(). Also the patch > is automatically freed, including the sysfs interface when the transition > to the disabled state is completed. > > As a result, there is never a disabled patch on the top of the stack. > Therefore we do not need to check the stack in __klp_enable_patch(). > And we could simplify the check in __klp_disable_patch(). > > Also the API and logic is much easier. It is enough to call > klp_enable_patch() in module_init() call. The patch patch can be disabled ^^^ s/patch patch/patch > by writing '0' into /sys/kernel/livepatch//enabled. Then the module > can be removed once the transition finishes and sysfs interface is freed. > > The only problem is how to free the structures and kobjects a safe way. > The operation is triggered from the sysfs interface. We could not put > the related kobject from there because it would cause lock inversion > between klp_mutex and kernfs locks, see kn->count lockdep map. > > This patch solved the problem by offloading the free task to > a workqueue. It is perfectly fine: > > + The patch cannot not longer be used in the livepatch operations. > > + The module could not be removed until the free operation finishes > and module_put() is called. > > + The operation is asynchronous already when the first > klp_try_complete_transition() fails and another call > is queued with a delay. > > Suggested-by: Josh Poimboeuf > Signed-off-by: Petr Mladek > --- Acked-by: Joe Lawrence > Documentation/livepatch/livepatch.txt| 137 ++--- > include/linux/livepatch.h| 5 +- > kernel/livepatch/core.c | 275 > +-- > kernel/livepatch/core.h | 2 + > kernel/livepatch/transition.c| 19 +- > samples/livepatch/livepatch-callbacks-demo.c | 13 +- > samples/livepatch/livepatch-sample.c | 13 +- > samples/livepatch/livepatch-shadow-fix1.c| 14 +- > samples/livepatch/livepatch-shadow-fix2.c| 14 +- > 9 files changed, 166 insertions(+), 326 deletions(-) > > diff --git a/Documentation/livepatch/livepatch.txt > b/Documentation/livepatch/livepatch.txt > index 2d7ed09dbd59..d849af312576 100644 > --- a/Documentation/livepatch/livepatch.txt > +++ b/Documentation/livepatch/livepatch.txt > > [ ... snip ... ] > > +5.1. Loading > + > > -5.1. Registration > -- > +The only reasonable way is to enable the patch when the livepatch kernel > +module is being loaded. For this, klp_enable_patch() has to be called > +in module_init() callback. There are two main reasons: s/in module_init()/in the module_init() > > [ ... snip ... ] > > +5.4. Removing > +- > > -At this stage, all the relevant sys-fs entries are removed and the patch > -is removed from the list of known patches. > +Module removal is only safe when there are no users of the underlying ^^ Could a reader confuse "underlying functions" for functions in the livepatching-core? Would "modified functions" or adding "(struct klp_func) " make this more specific? > +functions. This is the reason why the force feature permanently disables > +the removal. The forced tasks entered the functions but we cannot say ^ Same ambiguity here. > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index b71892693da5..1366dbb159ab 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepat
Re: [PATCH v14 06/11] livepatch: Use lists to manage patches, objects and functions
On Thu, Nov 29, 2018 at 10:44:26AM +0100, Petr Mladek wrote: > From: Jason Baron > > Currently klp_patch contains a pointer to a statically allocated array of > struct klp_object and struct klp_objects contains a pointer to a statically > allocated array of klp_func. In order to allow for the dynamic allocation > of objects and functions, link klp_patch, klp_object, and klp_func together > via linked lists. This allows us to more easily allocate new objects and > functions, while having the iterator be a simple linked list walk. > > The static structures are added to the lists early. It allows to add > the dynamically allocated objects before klp_init_object() and > klp_init_func() calls. Therefore it reduces the further changes > to the code. > > This patch does not change the existing behavior. > > Signed-off-by: Jason Baron > [pmla...@suse.com: Initialize lists before init calls] > Signed-off-by: Petr Mladek > Cc: Josh Poimboeuf > Cc: Jessica Yu > Cc: Jiri Kosina > Cc: Miroslav Benes > --- Acked-by: Joe Lawrence -- Joe
Re: [PATCH v14 07/11] livepatch: Add atomic replace
xisting one. > It is like when a new version of an application replaces an older one. > > This patch does the first step. It removes the replaced patches from > the list of patches. It is safe. The consistency model ensures that > they are not longer used. By other words, each process works only with ^^ s/not longer/no longer > the structures from klp_transition_patch. > > The removal is done by a special function. It combines actions done by > __disable_patch() and klp_complete_transition(). But it is a fast > track without all the transaction-related stuff. > > Signed-off-by: Jason Baron > [pmla...@suse.com: Split, reuse existing code, simplified] > Signed-off-by: Petr Mladek > Cc: Josh Poimboeuf > Cc: Jessica Yu > Cc: Jiri Kosina > Cc: Miroslav Benes > --- Acked-by: Joe Lawrence > diff --git a/Documentation/livepatch/livepatch.txt > b/Documentation/livepatch/livepatch.txt > index d849af312576..ba6e83a08209 100644 > --- a/Documentation/livepatch/livepatch.txt > +++ b/Documentation/livepatch/livepatch.txt > @@ -15,8 +15,9 @@ Table of Contents: > 5. Livepatch life-cycle > 5.1. Loading > 5.2. Enabling > - 5.3. Disabling > - 5.4. Removing > + 5.3. Replacing > + 5.4. Disabling > + 5.5. Removing > 6. Sysfs > 7. Limitations > > @@ -300,8 +301,12 @@ into three levels: > 5. Livepatch life-cycle > === > > -Livepatching can be described by four basic operations: > -loading, enabling, disabling, removing. > +Livepatching can be described by five basic operations: > +loading, enabling, replacing, disabling, removing. > + > +Where the replacing and the disabling operations are mutually > +exclusive. They have the same result for the given patch but > +not for the system. > > > 5.1. Loading > @@ -347,7 +352,23 @@ to '0'. > the "Consistency model" section. > > > -5.3. Disabling > +5.3. Replacing > +-- > + > +All enabled patches might get replaced by a cumulative patch that > +has the .replace flag set. > + > +Once the new patch is enabled and the 'transition" finishes then ^ ^ single quotes paired with double quotes -- Joe
Re: [PATCH v14 08/11] livepatch: Remove Nop structures when unused
On Thu, Nov 29, 2018 at 10:44:28AM +0100, Petr Mladek wrote: > Replaced patches are removed from the stack when the transition is > finished. It means that Nop structures will never be needed again > and can be removed. Why should we care? > > + Nop structures make false feeling that the function is patched ^^ re-wording suggestion: "give the impression" > even though the ftrace handler has no effect. > > + Ftrace handlers are not completely for free. They cause slowdown that re-wording suggesions: "free" or "do not come for free" > might be visible in some workloads. The ftrace-related slowdown might > actually be the reason why the function is not longer patched in ^^ s/not longer/no longer > the new cumulative patch. One would expect that cumulative patch > would allow to solve these problems as well. ^^ re-wording suggestion: "help solve" > > + Cumulative patches are supposed to replace any earlier version of > the patch. The amount of NOPs depends on which version was replaced. > This multiplies the amount of scenarios that might happen. > > One might say that NOPs are innocent. But there are even optimized > NOP instructions for different processor, for example, see ^ s/processor/processors > arch/x86/kernel/alternative.c. And klp_ftrace_handler() is much > more complicated. > > + It sounds natural to clean up a mess that is not longer needed. ^^ s/not longer/no longer > It could only be worse if we do not do it. > > This patch allows to unpatch and free the dynamic structures independently > when the transition finishes. > > The free part is a bit tricky because kobject free callbacks are called > asynchronously. We could not wait for them easily. Fortunately, we do > not have to. Any further access can be avoided by removing them from > the dynamic lists. > > Signed-off-by: Petr Mladek > --- Acked-by: Joe Lawrence -- Joe
Re: [PATCH v14 09/11] livepatch: Atomic replace and cumulative patches documentation
On Thu, Nov 29, 2018 at 10:44:29AM +0100, Petr Mladek wrote:
> User documentation for the atomic replace feature. It makes it easier
> to maintain livepatches using so-called cumulative patches.
>
> Signed-off-by: Petr Mladek
> ---
Acked-by: Joe Lawrence
> Documentation/livepatch/cumulative-patches.txt | 105
> +
> 1 file changed, 105 insertions(+)
> create mode 100644 Documentation/livepatch/cumulative-patches.txt
>
> diff --git a/Documentation/livepatch/cumulative-patches.txt
> b/Documentation/livepatch/cumulative-patches.txt
> new file mode 100644
> index ..a8089f7fe306
> --- /dev/null
> +++ b/Documentation/livepatch/cumulative-patches.txt
> @@ -0,0 +1,105 @@
> +===
> +Atomic Replace & Cumulative Patches
> +===
> +
> +There might be dependencies between livepatches. If multiple patches need
> +to do different changes to the same function(s) then we need to define
> +an order in which the patches will be installed. And function implementations
> +from any newer livepatch must be done on top of the older ones.
> +
> +This might become a maintenance nightmare. Especially if anyone would want
> +to remove a patch that is in the middle of the stack.
> +
> +An elegant solution comes with the feature called "Atomic Replace". It allows
> +to create so called "Cumulative Patches". They include all wanted changes
^
re-wording suggestion: "creation of"
> +from all older livepatches and completely replace them in one transition.
> +
> +Usage
> +-
> +
> +The atomic replace can be enabled by setting "replace" flag in struct
> klp_patch,
> +for example:
> +
> + static struct klp_patch patch = {
> + .mod = THIS_MODULE,
> + .objs = objs,
> + .replace = true,
> + };
> +
> +Such a patch is added on top of the livepatch stack when enabled.
> +
> +All processes are then migrated to use the code only from the new patch.
> +Once the transition is finished, all older patches are automatically
> +disabled and removed from the stack of patches.
> +
> +Ftrace handlers are transparently removed from functions that are no
> +longer modified by the new cumulative patch.
> +
> +As a result, the livepatch authors might maintain sources only for one
> +cumulative patch. It helps to keep the patch consistent while adding or
> +removing various fixes or features.
> +
> +Users could keep only the last patch installed on the system after
> +the transition to has finished. It helps to clearly see what code is
> +actually in use. Also the livepatch might then be seen as a "normal"
> +module that modifies the kernel behavior. The only difference is that
> +it can be updated at runtime without breaking its functionality.
> +
> +
> +Features
> +
> +
> +The atomic replace allows:
> +
> + + Atomically revert some functions in a previous patch while
> +upgrading other functions.
> +
> + + Remove eventual performance impact caused by core redirection
> +for functions that are no longer patched.
> +
> + + Decrease user confusion about stacking order and what code
> +is actually in use.
> +
> +
> +Limitations:
> +
> +
> + + Once the operation finishes, there is no straightforward way
> +to reverse it and restore the replaced patches atomically.
> +
> +A good practice is to set .replace flag in any released livepatch.
> +Then re-adding an older livepatch is equivalent to downgrading
> +to that patch. This is safe as long as the livepatches do _not_ do
> +extra modifications in (un)patching callbacks or in the module_init()
> +or module_exit() functions, see below.
> +
> +Also note that the replaced patch can be removed and loaded again
> +only when the transition was not forced.
> +
> +
> + + Only the (un)patching callbacks from the _new_ cumulative livepatch are
> +executed. Any callbacks from the replaced patches are ignored.
> +
> +In other words, the cumulative patch is responsible for doing any actions
> +that are necessary to properly replace any older patch.
> +
> +As a result, it might be dangerous to replace newer cumulative patches by
> +older ones. The old livepatches might not provide the necessary
> callbacks.
> +
> +This might be seen as a limitation in some scenarios. But it makes the
> life
s/the life/life
> +easier in many others. Only the new cumulative livepatch knows what
> +fixes/features are added
Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading conflicting patches
On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote:
> The atomic replace and cumulative patches were introduced as a more secure
> way to handle dependent patches. They simplify the logic:
>
> + Any new cumulative patch is supposed to take over shadow variables
> and changes made by callbacks from previous livepatches.
>
> + All replaced patches are discarded and the modules can be unloaded.
> As a result, there is only one scenario when a cumulative livepatch
> gets disabled.
>
> The different handling of "normal" and cumulative patches might cause
> confusion. It would make sense to keep only one mode. On the other hand,
> it would be rude to enforce using the cumulative livepatches even for
> trivial and independent (hot) fixes.
>
> This patch removes the stack of patches. The list of enabled patches
> is still needed but the ordering is not longer enforced.
^^
s/not longer/no longer
>
> Note that it is not possible to catch all possible dependencies. It is
> the responsibility of the livepatch authors to decide.
>
> Nevertheless this patch prevents having two patches for the same function
> enabled at the same time after the transition finishes. It might help
> to catch obvious mistakes. But more importantly, we do not need to
> handle situation when a patch in the middle of the function stack
s/handle situation/handle a situation
> (ops->func_stack) is being removed.
>
> Signed-off-by: Petr Mladek
> ---
Acked-by: Joe Lawrence
> Documentation/livepatch/cumulative-patches.txt | 11 ++
> Documentation/livepatch/livepatch.txt | 30 ---
> kernel/livepatch/core.c| 51
> --
> 3 files changed, 68 insertions(+), 24 deletions(-)
>
>
> [ ... snip ... ]
>
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index ba6e83a08209..3c150ab19b99 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -143,9 +143,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully
> supported by
> the kernel livepatching.
>
> The /sys/kernel/livepatch//transition file shows whether a patch
> -is in transition. Only a single patch (the topmost patch on the stack)
> -can be in transition at a given time. A patch can remain in transition
> -indefinitely, if any of the tasks are stuck in the initial patch state.
> +is in transition. Only a single patch can be in transition at a given
> +time. A patch can remain in transition indefinitely, if any of the tasks
> +are stuck in the initial patch state.
>
> A transition can be reversed and effectively canceled by writing the
> opposite value to the /sys/kernel/livepatch//enabled file while
> @@ -329,9 +329,10 @@ The livepatch gets enabled by calling klp_enable_patch()
> typically from
> module_init() callback. The system will start using the new implementation
> of the patched functions at this stage.
>
> -First, the addresses of the patched functions are found according to their
> -names. The special relocations, mentioned in the section "New functions",
> -are applied. The relevant entries are created under
> +First, possible conflicts are checked for non-cummulative patches with
^^^
s/non-cummulative/non-cumulative
>
> [ ... snip ... ]
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 0ce752e9e8bb..d8f6f49ac6b3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -122,6 +122,47 @@ static struct klp_object *klp_find_object(struct
> klp_patch *patch,
> return NULL;
> }
>
> +static int klp_check_obj_conflict(struct klp_patch *patch,
> + struct klp_object *old_obj)
> +{
> + struct klp_object *obj;
> + struct klp_func *func, *old_func;
> +
> + obj = klp_find_object(patch, old_obj);
> + if (!obj)
> + return 0;
> +
> + klp_for_each_func(old_obj, old_func) {
> + func = klp_find_func(obj, old_func);
> + if (!func)
> + continue;
> +
> + pr_err("Function '%s,%lu' in object '%s' has already been
> livepatched.\n",
> +func->old_name, func->old_sympos ? func->old_sympos : 1,
> +obj->name ? obj->name : "vmlinux");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Should we add a self-test check for this condition? Let me know and I
can give it a go if you want to include with v15.
-- Joe
Re: [PATCH v14 11/11] selftests/livepatch: introduce tests
On Thu, Nov 29, 2018 at 10:44:31AM +0100, Petr Mladek wrote: > From: Joe Lawrence > > Add a few livepatch modules and simple target modules that the included > regression suite can run tests against: > > - basic livepatching (multiple patches, atomic replace) > - pre/post (un)patch callbacks > - shadow variable API > > Signed-off-by: Joe Lawrence > Signed-off-by: Petr Mladek > --- Acked-by: Joe Lawrence > > [ ... snip ... ] > > diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh > b/tools/testing/selftests/livepatch/test-callbacks.sh > new file mode 100755 > index ..8d74c815bb8d > --- /dev/null > +++ b/tools/testing/selftests/livepatch/test-callbacks.sh > @@ -0,0 +1,587 @@ > > [ ... snip ... ] > > +# TEST: atomic replace > +# > +# Load multiple livepatches, but the second as an 'atomic-replace' > +# patch. When the latter laods, the original livepatch should be ^ s/laods/loads -- Joe
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 11/29/2018 04:44 AM, Petr Mladek wrote: > Hi, > > I have an updated present for your mailboxes. > > The atomic replace allows to create cumulative patches. They > are useful when you maintain many livepatches and want to remove > one that is lower on the stack. In addition it is very useful when > more patches touch the same function and there are dependencies > between them. > > All the changes were simple in principle but they required quite > some refactoring again :-( IMHO, the biggest change is renaming > klp_init_lists() ->klp_init_patch_before_free(). It does all > init actions that need to succeed before klp_free() functions > can be safely called. The main motivation was the need to > initialize also the new .kobj_alive flags. > > > Changes against v13: > > + Rename old_addr -> old_func instead of new_func -> new_addr. [Josh] > > + Do not add the helper macros to define structures. [Miroslav, Josh] > > + Add custom kobj_alive flag to reliably handle kobj state. [Miroslav] > Aside: I don't suppose that this could ever be folded into the kobject code/data structure itself? This seems like a common problem that kobj-users will need to solve like this. > + Avoid renaming .forced flag to .module_put by calling klp_free > functions only with taken module reference. [Josh] > > + Use list_add_tail() instead of list_add() when updating the dynamic > lists of klp_object and klp_func structures. Note that this > required also updating the order of messages from the pre/post > callbacks in the selftest. [Josh, Miroslav] > Updated self-tests ran fine for me, thanks for updating. > + Do not unnecessarily initialize ret variable in klp_add_nops(). [Miroslav] > > + Got rid of klp_discard_replaced_stuff(). [Josh] > > + Updated commit messages, comments and documentation, especially > the section "Livepatch life-cycle" [Josh, Miroslav] Thank you for adding/revising this part. It was pretty clear and it helped to read this before going through the individual patches. I don't have many code comments as the changes appear to safely and correctly do what the say. (We are at v14 after all :) I mainly compared the text and comments to the implementation and noted typos (marked by substitution s/old/new) and awkward wordings (marked by "re-wording suggestion"). That said, I ack'd each patch as I wouldn't want these to hold up the patchset. Thanks, -- Joe
Re: [PATCH v14 03/11] livepatch: Consolidate klp_free functions
On 12/06/2018 03:15 AM, Petr Mladek wrote:
> On Wed 2018-12-05 14:02:20, Joe Lawrence wrote:
>> On Thu, Nov 29, 2018 at 10:44:23AM +0100, Petr Mladek wrote:
>>> The code for freeing livepatch structures is a bit scattered and tricky:
>>>
>>> [ ... snip ... ]
>>>
>>> +static int klp_init_patch(struct klp_patch *patch)
>>> +{
>>> + struct klp_object *obj;
>>> + int ret;
>>> +
>>> + mutex_lock(&klp_mutex);
>>> +
>>> + ret = klp_init_patch_before_free(patch);
>>> if (ret) {
>>> mutex_unlock(&klp_mutex);
>>> return ret;
>>> }
>>>
>>
>> I believe klp_init_patch_before_free() accumulates more responsibilities
>> later in the patchset, but I'll ask here: does it really need the
>> klp_mutex since it looks to be operating only on the klp_patch, its
>> objects and functions?
>
> I do not have a strong opinion about it.
>
> On one hand, we are manipulating all the structures and should prevent
> any parallel use. On the other hand, the rest of the code will not
> touch the patch until it is added into klp_patches list or until
> the sysfs interface is created.
>
> If you think that it might cause false expectations and confusions
> then I could move it out of the lock.
I didn't find it confusing, nor is it performance limiting. I figured I
would point it out in case there was some reason for the mutex that I
had missed.
> Well, in the final version we need to call klp_check_patch_conflict()
> under the mutex.
That's right.
-- Joe
Re: [PATCH v14 00/11] livepatch: Atomic replace feature
On 12/06/2018 07:37 AM, Petr Mladek wrote: > On Thu 2018-12-06 11:15:55, Petr Mladek wrote: >> On Thu 2018-12-06 10:32:00, Miroslav Benes wrote: >>> > I don't have many code comments as the changes appear to safely and > correctly do what the say. (We are at v14 after all :) I mainly > compared the text and comments to the implementation and noted typos > (marked by substitution s/old/new) and awkward wordings (marked by > "re-wording suggestion"). That said, I ack'd each patch as I wouldn't > want these to hold up the patchset. Thanks a lot both you and Miroslav for the review. I'll give it some more days before I prepare v15. I wonder if Josh could find some cycle to look at it at least from the top level. >>> >>> For what is worth, I'm fine with all the changes Joe proposed and you can >>> preserve my acks there. >> >> We need enough time for bikeshedding about comments and documentation. >> I need some time to recover. And I still hope that Josh can give it >> a spin. > > Please, forget the above paragraph. I did not get Mirek's one correctly. > I understood it as "Why do we need to wait? You have two acks. > Jump on it and send us v15 immediately." > No worries :) I usually interpret language bikeshedding as an indication that the code is just about done. BTW, I think Josh will be back online next week, as he is out of the office this week. -- Joe
Re: [PATCH v14 05/11] livepatch: Simplify API by removing registration step
On 12/06/2018 05:14 AM, Petr Mladek wrote: > On Thu 2018-12-06 10:23:40, Miroslav Benes wrote: >> On Thu, 6 Dec 2018, Petr Mladek wrote: >> >>> On Wed 2018-12-05 14:32:53, Joe Lawrence wrote: >>>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >>>>> index 972520144713..e01dfa3b58d2 100644 >>>>> --- a/kernel/livepatch/core.c >>>>> +++ b/kernel/livepatch/core.c >>>>> @@ -45,7 +45,7 @@ >>>>> */ >>>>> DEFINE_MUTEX(klp_mutex); >>>>> >>>>> -/* Registered patches */ >>>>> +/* Actively used patches. */ >>>>> LIST_HEAD(klp_patches); >>>> >>>> By itself, this comment makes me wonder if there are un-active and/or >>>> un-used patches that I need to worry about. After this patchset, >>>> klp_patches will include patches that have been enabled and those that >>>> have been replaced, but the replacement transition is still in progress. >>>> >>>> If that sounds accurate, how about adding to the comment: >>>> >>>> /* Actively used patches: enabled or replaced and awaiting transition */ >>> >>> The replaced patches are not in the list. This is why I used the word >>> "actively". >> After writing out my suggestion I realized that's why you chose "actively" and almost erased my comment. I think the extra text would help a fresh reader of the code, so ... >> The replaced patches are removed in klp_discard_replaced_patches(), which >> is called from klp_complete_transition(). Joe is right. The patches are in >> the list if a transition is still in progress. > > These are patches that are being replaced. The replaced (after the > transition finishes) are not in the list. > > By other word, Joe's text could be understand that replaced patches > will never get removed from the list. > > So, is the text below acceptable? > > /* > * Actively used patches: enabled or in transition. Note that replaced > * or disabled patches are not listed even though the related kernel > * module still can be loaded. > */ Yes this works and is more accurate than my original suggestion. Thanks, -- Joe
double clock sched_features warnings msgs
Hi Peter, A QA tester here has noticed that he can regularly provoke a "rq->clock_update_flags & RQCF_UPDATED" warning when echo'ing into the debugfs sched_features file: % echo WARN_DOUBLE_CLOCK > /sys/kernel/debug/sched_features ... [ cut here ] rq->clock_update_flags & RQCF_UPDATED WARNING: CPU: 31 PID: 2052 at kernel/sched/core.c:203 update_rq_clock+0xdc/0x120 Modules linked in: sunrpc intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass ipmi_ssif crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate intel_uncore pcspkr sg intel_rapl_perf ipmi_si iTCO_wdt iTCO_vendor_support ioatdma ipmi_devintf wmi i2c_i801 ipmi_msghandler lpc_ich ip_tables xfs libcrc32c sr_mod cdrom sd_mod mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm isci drm igb libsas ahci libahci ptp scsi_transport_sas crc32c_intel libata pps_core dca i2c_algo_bit i2c_core dm_mirror dm_region_hash dm_log dm_mod CPU: 31 PID: 2052 Comm: (crond) Not tainted 4.18.0 #1 Hardware name: Intel Corporation SandyBridge Platform/To be filled by O.E.M., BIOS RMLSDP.86I.R2.28.D690.1306271008 06/27/2013 RIP: 0010:update_rq_clock+0xdc/0x120 Code: a8 04 0f 84 65 ff ff ff 80 3d 31 1a 11 01 00 0f 85 58 ff ff ff 48 c7 c7 f0 38 e6 81 31 c0 c6 05 1b 1a 11 01 01 e8 74 3d fd ff <0f> 0b 8b 83 b0 09 00 00 e9 36 ff ff ff e8 72 75 f8 ff 66 90 4c 8b RSP: 0018:c90004e13e90 EFLAGS: 00010086 RAX: RBX: 88042ea220c0 RCX: 0006 RDX: RSI: 0096 RDI: 88042eed6990 RBP: R08: 0025 R09: 0550 R10: R11: R12: 880429bb9200 R13: 880425264e00 R14: 000220c0 R15: FS: 7f5bb544c940() GS:88042eec() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 7f5bb3f59d40 CR3: 0004264e4004 CR4: 001606e0 Call Trace: online_fair_sched_group+0x53/0x110 sched_autogroup_create_attach+0xba/0x170 ksys_setsid+0xe7/0x100 __ia32_sys_setsid+0xa/0x10 do_syscall_64+0x5b/0x180 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f5bb3c42107 Code: 73 01 c3 48 8b 0d 89 fd 2f 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 70 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 59 fd 2f 00 f7 d8 64 89 01 48 RSP: 002b:7ffd3fa6a638 EFLAGS: 0246 ORIG_RAX: 0070 RAX: ffda RBX: 557397d52b68 RCX: 7f5bb3c42107 RDX: 7f5bb3f427b8 RSI: RDI: 000f RBP: 7ffd3fa6a8a0 R08: 0001 R09: 557397ec84a5 R10: 003b R11: 0246 R12: 1fff4fe9a99c R13: 7ffd3fa6a670 R14: R15: 7ffd3fa6aa80 ---[ end trace b5859b81be7e7db1 ]--- WARN_DOUBLE_CLOCK's definition in kernel/sched/features.h states that it is "Default disabled because the annotations are not complete". Should we expect such warnings (false positives?) until some further work is complete? Just trying to figure out if these msgs are interesting or not. Thanks, -- Joe
Re: [PATCH 2/2 V2] livepatch: handle kzalloc failure properly
On 12/13/2018 09:05 AM, Nicholas Mc Guire wrote:
> kzalloc() return should be checked. On dummy_alloc() failing
> in kzalloc() NULL should be returned.
>
> Signed-off-by: Nicholas Mc Guire
> ---
>
> Problem was located with an experimental coccinelle script
>
> V2: returning NULL is ok but not without cleanup - thanks to
> Petr Mladek for catching this.
>
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y, SAMPLE_LIVEPATCH=y
> (with a number of unrelated sparse warnings on symbols not being static)
>
> Patch is against 4.20-rc6 (localversion-next is next-20181213)
>
> samples/livepatch/livepatch-shadow-mod.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/samples/livepatch/livepatch-shadow-mod.c
> b/samples/livepatch/livepatch-shadow-mod.c
> index 4c54b25..4aa8a88 100644
> --- a/samples/livepatch/livepatch-shadow-mod.c
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> @@ -118,6 +118,10 @@ noinline struct dummy *dummy_alloc(void)
>
> /* Oops, forgot to save leak! */
> leak = kzalloc(sizeof(int), GFP_KERNEL);
> + if (!leak) {
> + kfree(d);
> + return NULL;
> + }
>
> pr_info("%s: dummy @ %p, expires @ %lx\n",
> __func__, d, d->jiffies_expire);
>
Hi Nicholas,
Thanks for finding and fixing these up... can we either squash these two
patches into a single commit or give them unique subject lines? Code
looks good (including Petr's suggested fix) otherwise.
-- Joe
Re: [PATCH 2/2 V2] livepatch: handle kzalloc failure properly
On 12/13/2018 10:39 AM, Nicholas Mc Guire wrote:
> On Thu, Dec 13, 2018 at 09:14:18AM -0500, Joe Lawrence wrote:
>> On 12/13/2018 09:05 AM, Nicholas Mc Guire wrote:
>>> kzalloc() return should be checked. On dummy_alloc() failing
>>> in kzalloc() NULL should be returned.
>>>
>>> Signed-off-by: Nicholas Mc Guire
>>> ---
>>>
>>> Problem was located with an experimental coccinelle script
>>>
>>> V2: returning NULL is ok but not without cleanup - thanks to
>>> Petr Mladek for catching this.
>>>
>>> Patch was compile tested with: x86_64_defconfig + FTRACE=y
>>> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y, SAMPLE_LIVEPATCH=y
>>> (with a number of unrelated sparse warnings on symbols not being static)
>>>
>>> Patch is against 4.20-rc6 (localversion-next is next-20181213)
>>>
>>> samples/livepatch/livepatch-shadow-mod.c | 4
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/samples/livepatch/livepatch-shadow-mod.c
>>> b/samples/livepatch/livepatch-shadow-mod.c
>>> index 4c54b25..4aa8a88 100644
>>> --- a/samples/livepatch/livepatch-shadow-mod.c
>>> +++ b/samples/livepatch/livepatch-shadow-mod.c
>>> @@ -118,6 +118,10 @@ noinline struct dummy *dummy_alloc(void)
>>>
>>> /* Oops, forgot to save leak! */
>>> leak = kzalloc(sizeof(int), GFP_KERNEL);
>>> + if (!leak) {
>>> + kfree(d);
>>> + return NULL;
>>> + }
>>>
>>> pr_info("%s: dummy @ %p, expires @ %lx\n",
>>> __func__, d, d->jiffies_expire);
>>>
>>
>> Hi Nicholas,
>>
>> Thanks for finding and fixing these up... can we either squash these two
>> patches into a single commit or give them unique subject lines? Code
>> looks good (including Petr's suggested fix) otherwise.
>>
> yup - makes sense to pop it into a single patch - I assumed that this
> would not be acceptable - so I actually split it up :)
> I´ll send a V3 then.
I don't know if there is a hard rule, but I always thought that unique
subject lines were desired to avoid grep/search confusion.
As far as one or two commits, I'd prefer a single commit since these are
so small. Personal preference, you could just say that you're fixing
samples/livepatch as a whole.
>
> BTW: wanted to fix up the sparse warnings but I think thats not going
> to be that simple as the functions/structs sparse complains about
> are actually being shared:
Ok, these are welcome too, separate commit...
> CHECK samples/livepatch/livepatch-shadow-fix1.c
> samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
> 'livepatch_fix1_dummy
> alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
> 'livepatch_fix1_dummy
> free' was not declared. Should it be static?
>
> CHECK samples/livepatch/livepatch-shadow-mod.c
> samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol 'dummy_list'
> was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
> 'dummy_list_mutex' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
> 'dummy_alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol 'dummy_free'
> was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
> 'dummy_check' was not declared. Should it be static?
>
> so to clean that appropriate declarations should probably
> go into a .h file. Technically its maybe not important as this
> is not production code - it would though be nice if sample
> code is sparse/smatch/cocci clean.
>
> would it be acceptable to clean this up with an additional
> livepatch-shadow-mod.h ?
I'm not a C language expert, but as I understand it: static functions
are only a namespacing game for the compiler. So I think it is safe to
pass around and call function pointers to static functions between
compilation units. At least I see this throughout the kernel, so that
is my assumption :)
-- Joe
Re: [PATCH 2/2 V2] livepatch: handle kzalloc failure properly
On 12/13/2018 01:51 PM, Nicholas Mc Guire wrote:
> On Thu, Dec 13, 2018 at 11:39:25AM -0500, Joe Lawrence wrote:
>> On 12/13/2018 10:39 AM, Nicholas Mc Guire wrote:
>>> On Thu, Dec 13, 2018 at 09:14:18AM -0500, Joe Lawrence wrote:
>>>> On 12/13/2018 09:05 AM, Nicholas Mc Guire wrote:
>>>>> kzalloc() return should be checked. On dummy_alloc() failing
>>>>> in kzalloc() NULL should be returned.
>>>>>
>>>>> Signed-off-by: Nicholas Mc Guire
>>>>> ---
>>>>>
>>>>> Problem was located with an experimental coccinelle script
>>>>>
>>>>> V2: returning NULL is ok but not without cleanup - thanks to
>>>>> Petr Mladek for catching this.
>>>>>
>>>>> Patch was compile tested with: x86_64_defconfig + FTRACE=y
>>>>> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y, SAMPLE_LIVEPATCH=y
>>>>> (with a number of unrelated sparse warnings on symbols not being static)
>>>>>
>>>>> Patch is against 4.20-rc6 (localversion-next is next-20181213)
>>>>>
>>>>> samples/livepatch/livepatch-shadow-mod.c | 4
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/samples/livepatch/livepatch-shadow-mod.c
>>>>> b/samples/livepatch/livepatch-shadow-mod.c
>>>>> index 4c54b25..4aa8a88 100644
>>>>> --- a/samples/livepatch/livepatch-shadow-mod.c
>>>>> +++ b/samples/livepatch/livepatch-shadow-mod.c
>>>>> @@ -118,6 +118,10 @@ noinline struct dummy *dummy_alloc(void)
>>>>>
>>>>> /* Oops, forgot to save leak! */
>>>>> leak = kzalloc(sizeof(int), GFP_KERNEL);
>>>>> + if (!leak) {
>>>>> + kfree(d);
>>>>> + return NULL;
>>>>> + }
>>>>>
>>>>> pr_info("%s: dummy @ %p, expires @ %lx\n",
>>>>> __func__, d, d->jiffies_expire);
>>>>>
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Thanks for finding and fixing these up... can we either squash these two
>>>> patches into a single commit or give them unique subject lines? Code
>>>> looks good (including Petr's suggested fix) otherwise.
>>>>
>>> yup - makes sense to pop it into a single patch - I assumed that this
>>> would not be acceptable - so I actually split it up :)
>>> I´ll send a V3 then.
>>
>> I don't know if there is a hard rule, but I always thought that unique
>> subject lines were desired to avoid grep/search confusion.
>>
> the duplicated subjectline was my mistake
>
>> As far as one or two commits, I'd prefer a single commit since these are
>> so small. Personal preference, you could just say that you're fixing
>> samples/livepatch as a whole.
>>
>>>
>>> BTW: wanted to fix up the sparse warnings but I think thats not going
>>> to be that simple as the functions/structs sparse complains about
>>> are actually being shared:
>>
>> Ok, these are welcome too, separate commit...
>>
>>> CHECK samples/livepatch/livepatch-shadow-fix1.c
>>> samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
>>> 'livepatch_fix1_dummy
>>> alloc' was not declared. Should it be static?
>>> samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
>>> 'livepatch_fix1_dummy
>>> free' was not declared. Should it be static?
>>>
>>> CHECK samples/livepatch/livepatch-shadow-mod.c
>>> samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol 'dummy_list'
>>> was not declared. Should it be static?
>>> samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
>>> 'dummy_list_mutex' was not declared. Should it be static?
>>> samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
>>> 'dummy_alloc' was not declared. Should it be static?
>>> samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol
>>> 'dummy_free' was not declared. Should it be static?
>>> samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
>>> 'dummy_check' was not declared. Should it be static?
>>>
>>> so to clean that appropriate declarations should probably
>>> go into a .h file. Technically its maybe not important as this
>>> is not production code
Re: [PATCH V2] livepatch: fix non-static warnings
On 12/18/2018 03:49 AM, Miroslav Benes wrote: > On Mon, 17 Dec 2018, Joe Lawrence wrote: > >> I'm just being picky about its documentation and how we should note its >> usage in the v3 patch. Think that s/__noclone/used/g of the v2 commit >> message would be sufficient? > > We could rephrase it. After all it is not only about symbol names in the > symbol table. The traceable/patchable code has to be present... > > "Sparse reported warnings about non-static symbols. For the variables > a simple static attribute is fine - for the functions referenced by > livepatch via klp_func the symbol-names must be unmodified in the > symbol table and the patchable code has to be emitted. > > Attach __used attribute to the shared statically declared functions." > > ? That works for me. -- Joe
Re: [PATCH 1/2] livepatch: fix non-static warnings
On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> Sparse reported warnings about non-static symbols. For the variables a
> simple static attribute is fine - for those symbols referenced by
> livepatch via klp_func the symbol-names must be unmodified in the
> relocation table - to resolve this the __noclone attribute (as
^^
nit: symbol table
> suggested by Joe Lawrence ) is used
> for the statically declared functions.
>
> Signed-off-by: Nicholas Mc Guire
> Link: https://lkml.org/lkml/2018/12/13/827
> ---
>
> sparse reported the following warnings:
>
> CHECK samples/livepatch/livepatch-shadow-fix1.c
> samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
> 'livepatch_fix1_dummy alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
> 'livepatch_fix1_dummy free' was not declared. Should it be static?
>
> CHECK samples/livepatch/livepatch-shadow-mod.c
> samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
> 'dummy_list' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
> 'dummy_list_mutex' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
> 'dummy_alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol
> 'dummy_free' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
> 'dummy_check' was not declared. Should it be static?
>
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> SAMPLE_LIVEPATCH=y
>
> Patch was runtested on an Intel i3 with:
>insmod samples/livepatch/livepatch-shadow-mod.ko
>insmod samples/livepatch/livepatch-shadow-fix1.ko
>insmod samples/livepatch/livepatch-shadow-fix2.ko
>echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
>echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
>rmmod livepatch-shadow-fix2
>rmmod livepatch-shadow-fix1
>rmmod livepatch-shadow-mod
> and dmesg output checked.
>
> Patch is against 4.20-rc6 (localversion-next is next-20181214)
Great testing notes, thanks for including these!
> samples/livepatch/livepatch-shadow-fix1.c | 4 ++--
> samples/livepatch/livepatch-shadow-mod.c | 16 +++-
> 2 files changed, 13 insertions(+), 7 deletions(-)
Almost. We should only need to annotate with __noclone for those
function definitions which the samples will be patching. As the commit
message says, these correlate to klp_func.old_name functions found in
klp_object.name objects (.ko modules or NULL for vmlinux).
For the functions defined in samples/livepatch/*.c those would be:
livepatch-callbacks-busymod.c :: busymod_work_func()
livepatch-shadow-mod.c :: dummy_alloc()
livepatch-shadow-mod.c :: dummy_free()
livepatch-shadow-mod.c :: dummy_check()
So even though livepatch-shadow-fix2 further refines
livepatch-shadow-fix1, the livepatch core is going to redirect the
original dummy_*() calls defined by livepatch-shadow-mod.c in both fix1
and fix2 cases. Ie, the fixes modules aren't patched, only the original.
>
> diff --git a/samples/livepatch/livepatch-shadow-fix1.c
> b/samples/livepatch/livepatch-shadow-fix1.c
> index 49b1355..eaab10f 100644
> --- a/samples/livepatch/livepatch-shadow-fix1.c
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> @@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data,
> void *ctor_data)
> return 0;
> }
>
> -struct dummy *livepatch_fix1_dummy_alloc(void)
> +static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
> {
> struct dummy *d;
> void *leak;
> @@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj,
> void *shadow_data)
>__func__, d, *shadow_leak);
> }
>
> -void livepatch_fix1_dummy_free(struct dummy *d)
> +static __noclone void livepatch_fix1_dummy_free(struct dummy *d)
> {
> void **shadow_leak;
>
> diff --git a/samples/livepatch/livepatch-shadow-mod.c
> b/samples/livepatch/livepatch-shadow-mod.c
> index 4c54b25..0a72bc2 100644
> --- a/samples/livepatch/livepatch-shadow-mod.c
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> @@ -30,6 +30,11 @@
> * memory leak, please load these modules at your own risk -- some
> * amount of memory may leaked before the bug is patched.
> *
> + * NOTE - the __noclone attribute to those functions that are to be
> + * shared with other modules while being declared static. As livepatch
> + *
Re: [PATCH 2/2] livepatch: check kzalloc return values
On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> kzalloc() return should always be checked - notably in example code
> where this may be seen as reference. On failure of allocation in
> livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
> allocation is freed (thanks to Petr Mladek for
> catching this) and NULL returned.
>
> Signed-off-by: Nicholas Mc Guire
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")
> ---
>
> Problem located with an experimental coccinelle script
>
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> SAMPLE_LIVEPATCH=y
>
> Patch is against 4.20-rc6 (localversion-next is next-20181214)
> on top of 0001-livepatch-fix-non-static-warnings.patch
>
> samples/livepatch/livepatch-shadow-fix1.c | 5 +
> samples/livepatch/livepatch-shadow-mod.c | 4
> 2 files changed, 9 insertions(+)
>
> diff --git a/samples/livepatch/livepatch-shadow-fix1.c
> b/samples/livepatch/livepatch-shadow-fix1.c
> index eaab10f..1974eb5 100644
> --- a/samples/livepatch/livepatch-shadow-fix1.c
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> @@ -89,6 +89,11 @@ static __noclone struct dummy
> *livepatch_fix1_dummy_alloc(void)
>* pointer to handle resource release.
>*/
> leak = kzalloc(sizeof(int), GFP_KERNEL);
> + if (!leak) {
> + kfree(d);
> + return NULL;
> + }
> +
> klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
>shadow_leak_ctor, leak);
>
> diff --git a/samples/livepatch/livepatch-shadow-mod.c
> b/samples/livepatch/livepatch-shadow-mod.c
> index dc69da0..b4ece36 100644
> --- a/samples/livepatch/livepatch-shadow-mod.c
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> @@ -123,6 +123,10 @@ static __noclone noinline struct dummy *dummy_alloc(void)
>
> /* Oops, forgot to save leak! */
> leak = kzalloc(sizeof(int), GFP_KERNEL);
> + if (!leak) {
> + kfree(d);
> + return NULL;
> + }
>
> pr_info("%s: dummy @ %p, expires @ %lx\n",
> __func__, d, d->jiffies_expire);
>
Patch v2 2/2 looks good, thanks for combining.
Acked-by: Joe Lawrence
-- Joe
Re: [PATCH 1/2] livepatch: fix non-static warnings
On 12/14/2018 04:51 PM, Josh Poimboeuf wrote: > On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote: >> BTW, Petr/Miroslav/Josh, should we be annotating the selftests in >> similar fashion? > > Probably so. > Just to clarify for Nicholas, the self-tests we're referring to are part of another patch series currently under review -- no need to worry about them for the changes here. Thanks, -- Joe
Re: [PATCH 1/2] livepatch: fix non-static warnings
On Sat, Dec 15, 2018 at 09:50:52AM +0100, Nicholas Mc Guire wrote:
> On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote:
> > On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> > > Sparse reported warnings about non-static symbols. For the variables a
> > > simple static attribute is fine - for those symbols referenced by
> > > livepatch via klp_func the symbol-names must be unmodified in the
> > > relocation table - to resolve this the __noclone attribute (as
> > ^^
> > nit: symbol table
>
> that should have been relocation section as described in
> Documentation/livepatch/module-elf-format.txt - atleast that is how
> I currently undderstand the livepatch mechanism and its seperate
> relocation section.
>
Hi Nicholas,
Jessica can explain module-elf-format.txt in more detail, but the
highlight is that it outlines the format for _livepatch_ modules, in
this case livepatch-shadow-fix{1,2}.ko. The special relocations
detailed in that file are needed when livepatch modules reference
symbols defined elsewhere (vmlinux, other modules) that may not
ordinarily be visible (non-exported, local, etc.) to the module loader.
When livepatch modules register themselves on load, the livepatching
core needs to find the address of the _target_ to-be-patched function
using a kallsyms lookup. If that can't be found, the kernel will emit
an error, "livepatch: symbol 'dummy_free' not found in symbol table".
The call path is:
klp_register_patch
klp_init_object_loaded
klp_find_object_symbol
if you want to trace the execution path.
>
> thanks for your patience - so I did not yet understand how this really
> works together - will give it a rerun and repost a hopefully proper
> solution.
>
> thx!
> hofrat
Thanks for sticking with it and learning some livepatching internals
along the way.
-- Joe
Re: [PATCH V2] livepatch: fix non-static warnings
On 12/17/2018 07:03 AM, Miroslav Benes wrote:
> Hi,
>
> I'm sorry for being late to the party.
>
> On Sun, 16 Dec 2018, Nicholas Mc Guire wrote:
>
>> Sparse reported warnings about non-static symbols. For the variables
>> a simple static attribute is fine - for those symbols referenced by
>> livepatch via klp_func the symbol-names must be unmodified in the
>> symbol table - to resolve this the __noclone attribute is used
>> for the shared statically declared functions.
>>
>> Signed-off-by: Nicholas Mc Guire
>> Suggested-by: Joe Lawrence
>> Link: https://lkml.org/lkml/2018/12/13/827
>
> A nit, but I'd reorder the tags. Link, Suggested-by:, Signed-off-by:. Also
> it would be great if you used https://lkml.kernel.org/r/${Msg-ID}
> redirection.
>
>> ---
>>
>> V2: not all static functions shared need to carry the __noclone
>> attribute only those that need to be resolved at runtime by
>> livepatch - so drop the unnecessary __noclone attributes as
>> well as the Note on __noclone as suggested by Joe Lawrence
>> - thanks !
>
> I talked to Martin Jambor (GCC) and he suggested __attribute__((used)). It
> should be better than __noclone, which was reportedly implemented only for
> testing purposes (which is why it does not imply noinline, although
> inlining internally uses cloning). Newer gcc also has "noipa" attribute,
> but "used" would definitely be safe.
>
> Sorry for not responding earlier.
>
Hi Miroslav,
Thanks for following up on this. "noipa" would have been nice to use,
but as you say, is a newer gcc attribute.
Regarding "used" vs. "noclone", can we assume that "used" implies that
the function name remains unchanged?
The gcc online doc [1] speaks about ensuring that "code must be
emitted". This reads like it solves our
static-function-not-directly-referenced problem, but isn't explicit
about naming.
used
This attribute, attached to a function, means that code must be
emitted for the function even if it appears that the function is not
referenced. This is useful, for example, when the function is
referenced only in inline assembly.
Perhaps it's equivalent to a "I want to keep this function and leave
it's symbols alone" attribute. FWIW, I modified Nicholas' change on my
box to use "used" and it worked as Martin advertised, so it would get my
Ack.
I'm just being picky about its documentation and how we should note its
usage in the v3 patch. Think that s/__noclone/used/g of the v2 commit
message would be sufficient?
[1]
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noclone-function-attribute
Thanks,
-- Joe
Re: s390: runtime warning about pgtables_bytes
On Fri, Oct 12, 2018 at 05:08:33PM +0200, Martin Schwidefsky wrote: > On Thu, 11 Oct 2018 15:02:11 +0200 > Martin Schwidefsky wrote: > > > On Thu, 11 Oct 2018 18:04:12 +0800 > > Li Wang wrote: > > > > > When running s390 system with LTP/cve-2017-17052.c[1], the following BUG > > > is > > > came out repeatedly. > > > I remember this warning start from kernel-4.16.0 and now it still exist in > > > kernel-4.19-rc7. > > > Can anyone take a look? > > > > > > [ 2678.991496] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.001543] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.002453] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.003256] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.013689] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.024647] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.064408] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > [ 2679.133963] BUG: non-zero pgtables_bytes on freeing mm: 16384 > > > > > > [1]: > > > https://github.com/linux-test-project/ltp/blob/master/testcases/cve/cve-2017-17052.c > > > > > > > Confirmed, I see this bug with cvs-2017-17052 on my LPAR as well. > > I'll look into it. > > Ok, I think I understand the problem now. This is the patch I am testing > right now. It seems to fix the issue, but I had to change common mm > code for it. > -- > >From 9e3bc2e96930206ef1ece377e45224c51aca1799 Mon Sep 17 00:00:00 2001 > From: Martin Schwidefsky > Date: Fri, 12 Oct 2018 16:32:29 +0200 > Subject: [RFC][PATCH] s390/mm: fix mis-accounting of pgtable_bytes > > In case a fork or a clone system fails in copy_process and the error > handling does the mmput() at the bad_fork_cleanup_mm label, the following > warning messages will appear on the console: > > BUG: non-zero pgtables_bytes on freeing mm: 16384 > > The reason for that is the tricks we play with mm_inc_nr_puds() and > mm_inc_nr_pmds() in init_new_context(). > > A normal 64-bit process has 3 levels of page table, the p4d level and > the pud level are folded. On process termination the free_pud_range() > function in mm/memory.c will subtract 16KB from pgtable_bytes with a > mm_dec_nr_puds() call, but there actually is not really a pud table. > The s390 version of pud_free_tlb() recognized this an does nothing, > the region-3 table will be freed with the pgd_free() call later on. > But the mm_dec_nr_puds() is done unconditionally, to counter act this > the init_new_context() function has an extra mm_inc_nr_puds() call. > > Now with a failed fork or clone the free_pgtables() function is not > called, there is no mm_dec_nr_puds() but the mm_inc_nr_puds() has > been done which leads to the incorrect pgtable_bytes of 16384. > Nothing is broken by this, but the warning is annoying. > > To get rid of the warning drop the mm_inc_nr_pmds() & mm_inc_nr_puds() > calls from init_new_context(), introduce the mm_pmd_folded(), > pmd_pud_folded() and pmd_p4d_folded() helper, and add if-statements > to the functions mm_[inc|dec]_nr_[pmds|puds]. > > Signed-off-by: Martin Schwidefsky > --- > arch/s390/include/asm/mmu_context.h | 5 - > arch/s390/include/asm/pgalloc.h | 6 ++--- > arch/s390/include/asm/pgtable.h | 18 +++ > arch/s390/include/asm/tlb.h | 6 ++--- > include/linux/mm.h | 44 > - > 5 files changed, 62 insertions(+), 17 deletions(-) > > diff --git a/arch/s390/include/asm/mmu_context.h > b/arch/s390/include/asm/mmu_context.h > index dbd689d556ce..ccbb53e22024 100644 > --- a/arch/s390/include/asm/mmu_context.h > +++ b/arch/s390/include/asm/mmu_context.h > @@ -46,8 +46,6 @@ static inline int init_new_context(struct task_struct *tsk, > mm->context.asce_limit = STACK_TOP_MAX; > mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH | > _ASCE_USER_BITS | _ASCE_TYPE_REGION3; > - /* pgd_alloc() did not account this pud */ > - mm_inc_nr_puds(mm); > break; > case -PAGE_SIZE: > /* forked 5-level task, set new asce with new_mm->pgd */ > @@ -63,9 +61,6 @@ static inline int init_new_context(struct task_struct *tsk, > /* forked 2-level compat task, set new asce with new mm->pgd */ > mm->context.asce = __pa(mm->pgd) | _ASCE_TABLE_LENGTH | > _ASCE_USER_BITS | _ASCE_TYPE_SEGMENT; > - /* pgd_alloc() did not account this pmd */ > - mm_inc_nr_pmds(mm); > - mm_inc_nr_puds(mm); > } > crst_table_init((unsigned long *) mm->pgd, pgd_entry_type(mm)); > return 0; > diff --git a/arch/s390/include/asm/pgalloc.h b/arch/s390/include/asm/pgalloc.h > index f0f9bcf94c03..5ee733720a57 100644 > --- a/arch/s390/include/asm/pgalloc.h > +++ b/arch/s390/include/asm/pgalloc.h > @@ -36,11 +36,11 @@ static inline void crst_table_init(uns
Re: [PATCH v13 12/12] selftests/livepatch: introduce tests
On 10/15/2018 08:37 AM, Petr Mladek wrote:
> From: Joe Lawrence
>
> Add a few livepatch modules and simple target modules that the included
> regression suite can run tests against:
>
> - basic livepatching (multiple patches, atomic replace)
> - pre/post (un)patch callbacks
> - shadow variable API
>
> Signed-off-by: Joe Lawrence
> ---
>
> [ ... snip ... ]
>
> diff --git a/tools/testing/selftests/livepatch/functions.sh
> b/tools/testing/selftests/livepatch/functions.sh
> new file mode 100644
> index ..d448b115f06c
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/functions.sh
>
> [ ... snip ... ]
>
> +# disable_lp(modname) - disable a livepatch
> +#modname - module name to unload
> +function disable_lp() {
> + local mod="$1"
> +
> + log "% echo 0 > /sys/kernel/livepatch/$mod/enabled"
> + echo 0 > /sys/kernel/livepatch/"$mod"/enabled
> +
> + # Wait until the transition finishes and the livepatch gets
> + # removed from sysfs...
> + loop_until '[[ ! -e "/sys/kernel/livepatch/$mod" ]]' ||
> + die "failed to disable livepatch $mod"
small nit in case there is a v14:
Applying: selftests/livepatch: introduce tests
/root/linux/.git/rebase-apply/patch:1513: space before tab in indent.
# Wait until the transition finishes and the livepatch gets
And FWIW v13's selftests ran just fine on ppc64le over here.
-- Joe
Re: [PATCH] livepatch: Add KLP_IDLE state
On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote: > From: Wardenjohn > > In livepatch, using KLP_UNDEFINED is seems to be confused. > When kernel is ready, livepatch is ready too, which state is > idle but not undefined. What's more, if one livepatch process > is finished, the klp state should be idle rather than undefined. > > Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better > in reading and understanding. > --- > include/linux/livepatch.h | 1 + > kernel/livepatch/patch.c | 2 +- > kernel/livepatch/transition.c | 24 > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..c1c53cd5b227 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -19,6 +19,7 @@ > > /* task patch states */ > #define KLP_UNDEFINED-1 > +#define KLP_IDLE -1 Hi Wardenjohn, Quick question, does this patch intend to: - Completely replace KLP_UNDEFINED with KLP_IDLE - Introduce KLP_IDLE as an added, fourth potential state - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain conditions I ask because this patch leaves KLP_UNDEFINED defined and used in other parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and continues to use the same -1 enumeration. -- Joe
Re: [PATCH] livepatch: Add KLP_IDLE state
On 4/4/24 11:17, Petr Mladek wrote:
> On Tue 2024-04-02 09:52:31, Joe Lawrence wrote:
>> On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote:
>>> From: Wardenjohn
>>>
>>> In livepatch, using KLP_UNDEFINED is seems to be confused.
>>> When kernel is ready, livepatch is ready too, which state is
>>> idle but not undefined. What's more, if one livepatch process
>>> is finished, the klp state should be idle rather than undefined.
>>>
>>> Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better
>>> in reading and understanding.
>>> ---
>>> include/linux/livepatch.h | 1 +
>>> kernel/livepatch/patch.c | 2 +-
>>> kernel/livepatch/transition.c | 24
>>> 3 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>> index 9b9b38e89563..c1c53cd5b227 100644
>>> --- a/include/linux/livepatch.h
>>> +++ b/include/linux/livepatch.h
>>> @@ -19,6 +19,7 @@
>>>
>>> /* task patch states */
>>> #define KLP_UNDEFINED -1
>>> +#define KLP_IDLE -1
>>
>> Hi Wardenjohn,
>>
>> Quick question, does this patch intend to:
>>
>> - Completely replace KLP_UNDEFINED with KLP_IDLE
>> - Introduce KLP_IDLE as an added, fourth potential state
>> - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain
>> conditions
>>
>> I ask because this patch leaves KLP_UNDEFINED defined and used in other
>> parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and
>> continues to use the same -1 enumeration.
>
> Having two names for the same state adds more harm than good.
>
> Honestly, neither "task->patch_state == KLP_UNDEFINED" nor "KLP_IDLE"
> make much sense.
>
> The problem is in the variable name. It is not a state of a patch.
> It is the state of the transition. The right solution would be
> something like:
>
> klp_target_state -> klp_transition_target_state
> task->patch_state -> task->klp_transition_state
> KLP_UNKNOWN -> KLP_IDLE
>
Yes, this is exactly how I think of these when reading the code. The
model starts to make a lot more sense once you look at it thru this lens :)
> But it would also require renaming:
>
> /proc//patch_state -> klp_transition_state
>
> which might break userspace tools => likely not acceptable.
>
>
> My opinion:
>
> It would be nice to clean this up but it does not look worth the
> effort.
>
Agreed. Instead of changing code and the sysfs interface, we could
still add comments like:
/* task patch transition target states */
#define KLP_UNDEFINED -1 /* idle, no transition in progress */
#define KLP_UNPATCHED0 /* transitioning to unpatched state */
#define KLP_PATCHED 1 /* transitioning to patched state */
/* klp transition target state */
static int klp_target_state = KLP_UNDEFINED;
struct task_struct = {
.patch_state= KLP_UNDEFINED, /* klp transition state */
Maybe just one comment is enough? Alternatively, we could elaborate in
Documentation/livepatch/livepatch.rst if it's really confusing.
Wardenjohn, since you're probably reading this code with fresh(er) eyes,
would any of the above be helpful?
--
Joe
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
Hi Lukas,
As mentioned offlist, reviewing and testing this is on my TODO list, but
here are some early notes ...
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote:
> Livepatches need to access external symbols which can't be handled
> by the normal relocation mechanism. It is needed for two types
> of symbols:
>
> + Symbols which can be local for the original livepatched function.
> The alternative implementation in the livepatch sees them
> as external symbols.
>
> + Symbols in modules which are exported via EXPORT_SYMBOL*(). They
> must be handled special way otherwise the livepatch module would
> depend on the livepatched one. Loading such livepatch would cause
> loading the other module as well.
>
> The address of these symbols can be found via kallsyms. Or they can
> be relocated using livepatch specific relocation sections as specified
> in Documentation/livepatch/module-elf-format.txt.
>
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
>
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
>
> Allow to annotate such external symbols in the livepatch by a macro
> KLP_RELOC_SYMBOL(). It will create symbol with all needed
> metadata. For example:
>
> extern char *saved_command_line \
> KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0);
Nit: should this be KLP_RELOC_SYMBOL_POS if it including the 0 position?
(Or KLP_RELOC_SYMBOL and omit the implied 0-th position.)
> diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
> --- /dev/null
> +++ b/scripts/livepatch/elf.c
> +static int update_shstrtab(struct elf *elf)
> +{
> + struct section *shstrtab, *sec;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + shstrtab = find_section_by_name(elf, ".shstrtab");
> + if (!shstrtab) {
> + WARN("can't find .shstrtab");
> + return -1;
> + }
> +
> + orig_size = new_size = shstrtab->size;
> +
> + list_for_each_entry(sec, &elf->sections, list) {
> + if (sec->sh.sh_name != ~0U)
> + continue;
> + new_size += strlen(sec->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)shstrtab->data, orig_size);
While reviewing klp-convert v7 [1], Alexey Dobriyan notes here,
"This code is called realloc(). :-)"
[1]
https://lore.kernel.org/live-patching/4ce29654-4e1e-4680-9c25-715823ff5e02@p183/
> +static int update_strtab(struct elf *elf)
> +{
> + struct section *strtab;
> + struct symbol *sym;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + strtab = find_section_by_name(elf, ".strtab");
> + if (!strtab) {
> + WARN("can't find .strtab");
> + return -1;
> + }
> +
> + orig_size = new_size = strtab->size;
> +
> + list_for_each_entry(sym, &elf->symbols, list) {
> + if (sym->sym.st_name != ~0U)
> + continue;
> + new_size += strlen(sym->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)strtab->data, orig_size);
I think Alexey's realloc suggestion would apply here, too.
> +static int write_file(struct elf *elf, const char *file)
> +{
> + int fd;
> + Elf *e;
> + GElf_Ehdr eh, ehout;
> + Elf_Scn *scn;
> + Elf_Data *data;
> + GElf_Shdr sh;
> + struct section *sec;
> +
> + fd = creat(file, 0664);
> + if (fd == -1) {
> + WARN("couldn't create %s", file);
> + return -1;
> + }
> +
> + e = elf_begin(fd, ELF_C_WRITE, NULL);
Alexy also found an ELF coding bug:
"elf_end() doesn't close descriptor, so there is potentially corrupted
data. There is no unlink() call if writes fail as well."
> +void elf_close(struct elf *elf)
> +{
> + struct section *sec, *tmpsec;
> + struct symbol *sym, *tmpsym;
> + struct rela *rela, *tmprela;
> +
> + list_for_each_entry_safe(sym, tmpsym, &elf->symbols, list) {
> + list_del(&sym->list);
> + free(sym);
> + }
> + list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
> + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> + list_del(&rela->list);
> + free(rela);
> + }
> +
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote: > Livepatches need to access external symbols which can't be handled > by the normal relocation mechanism. It is needed for two types > of symbols: > > + Symbols which can be local for the original livepatched function. > The alternative implementation in the livepatch sees them > as external symbols. > > + Symbols in modules which are exported via EXPORT_SYMBOL*(). They > must be handled special way otherwise the livepatch module would > depend on the livepatched one. Loading such livepatch would cause > loading the other module as well. > > The address of these symbols can be found via kallsyms. Or they can > be relocated using livepatch specific relocation sections as specified > in Documentation/livepatch/module-elf-format.txt. > > Currently, there is no trivial way to embed the required information as > requested in the final livepatch elf object. klp-convert solves this > problem by using annotations in the elf object to convert the relocation > accordingly to the specification, enabling it to be handled by the > livepatch loader. > > Given the above, create scripts/livepatch to hold tools developed for > livepatches and add source files for klp-convert there. > > Allow to annotate such external symbols in the livepatch by a macro > KLP_RELOC_SYMBOL(). It will create symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > Also add scripts/livepatch/klp-convert. The tool transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbol with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > klp-convert relies on libelf and on a list implementation. Add files > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf > interfacing layer and scripts/livepatch/list.h, which is a list > implementation. > > Update Makefiles to correctly support the compilation of the new tool, > update MAINTAINERS file and add a .gitignore file. > > [jpoim...@redhat.com: initial version] > Signed-off-by: Josh Poimboeuf > [joe.lawre...@redhat.com: clean-up and fixes] > Signed-off-by: Joe Lawrence > [lhru...@suse.cz: klp-convert code, minimal approach] > Signed-off-by: Lukas Hruska > Reviewed-by: Marcos Paulo de Souza > --- > MAINTAINERS | 1 + > include/linux/livepatch.h | 19 + > scripts/Makefile| 1 + > scripts/livepatch/.gitignore| 1 + > scripts/livepatch/Makefile | 5 + > scripts/livepatch/elf.c | 817 > scripts/livepatch/elf.h | 73 +++ > scripts/livepatch/klp-convert.c | 284 +++ > scripts/livepatch/klp-convert.h | 23 + > scripts/livepatch/list.h| 391 +++ > 10 files changed, 1615 insertions(+) > create mode 100644 scripts/livepatch/.gitignore > create mode 100644 scripts/livepatch/Makefile > create mode 100644 scripts/livepatch/elf.c > create mode 100644 scripts/livepatch/elf.h > create mode 100644 scripts/livepatch/klp-convert.c > create mode 100644 scripts/livepatch/klp-convert.h > create mode 100644 scripts/livepatch/list.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 130b8b0bd4f7..d2facc1f4e15 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12618,6 +12618,7 @@ F:include/uapi/linux/livepatch.h > F: kernel/livepatch/ > F: kernel/module/livepatch.c > F: samples/livepatch/ > +F: scripts/livepatch/ > F: tools/testing/selftests/livepatch/ > > LLC (802.2) > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..83bbcd1c43fd 100644 > --- a/include/linux/livepatch.h > +++ b/i
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, May 21, 2024 at 08:34:46AM +0200, Miroslav Benes wrote: > Hello, > > On Mon, 20 May 2024, zhang warden wrote: > > > > > > > > On May 20, 2024, at 14:46, Miroslav Benes wrote: > > > > > > Hi, > > > > > > On Mon, 20 May 2024, Wardenjohn wrote: > > > > > >> Livepatch module usually used to modify kernel functions. > > >> If the patched function have bug, it may cause serious result > > >> such as kernel crash. > > >> > > >> This is a kobject attribute of klp_func. Sysfs interface named > > >> "called" is introduced to livepatch which will be set as true > > >> if the patched function is called. > > >> > > >> /sys/kernel/livepatchcalled > > >> > > >> This value "called" is quite necessary for kernel stability > > >> assurance for livepatching module of a running system. > > >> Testing process is important before a livepatch module apply to > > >> a production system. With this interface, testing process can > > >> easily find out which function is successfully called. > > >> Any testing process can make sure they have successfully cover > > >> all the patched function that changed with the help of this interface. > > > > > > Even easier is to use the existing tracing infrastructure in the kernel > > > (ftrace for example) to track the new function. You can obtain much more > > > information with that than the new attribute provides. > > > > > > Regards, > > > Miroslav > > Hi Miroslav > > > > First, in most cases, testing process is should be automated, which make > > using existing tracing infrastructure inconvenient. > > could you elaborate, please? We use ftrace exactly for this purpose and > our testing process is also more or less automated. > > > Second, livepatch is > > already use ftrace for functional replacement, I don’t think it is a > > good choice of using kernel tracing tool to trace a patched function. > > Why? > > > At last, this attribute can be thought of as a state of a livepatch > > function. It is a state, like the "patched" "transition" state of a > > klp_patch. Adding this state will not break the state consistency of > > livepatch. > > Yes, but the information you get is limited compared to what is available > now. You would obtain the information that a patched function was called > but ftrace could also give you the context and more. > > It would not hurt to add a new attribute per se but I am wondering about > the reason and its usage. Once we have it, the commit message should be > improved based on that. > I agree with Miroslav about using ftrace, or Dan in the other thread about using gcov if even more granular coverage is needed. Admittedly, I would have to go find documentation to remember how to do either, but at least I'd be using well-tested facilities designed for this exact purpose. Adding these attributes to livepatch sysfs would be expedient and probably easier for us to use, but imposes a recurring burden on us to maintain and test (where is the documentation and kselftest for this new interface?). Or, we could let the other tools handle all of that for us. Perhaps if someone already has an off-the-shelf script that is using ftrace to monitor livepatched code, it could be donated to Documentation/livepatch/? I can ask our QE folks if they have something like this. Regards, -- Joe
Re: [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
On 5/31/24 07:23, Miroslav Benes wrote: > Hi, > > On Tue, 21 Jul 2020, Joe Lawrence wrote: > >> In light of [PATCH] Revert "kbuild: use -flive-patching when >> CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers >> and explanation of the impact compiler optimizations have on >> livepatching. >> >> The first commit provides detailed explanations and examples. The list >> was taken mostly from Miroslav's LPC talk a few years back. This is a >> bit rough, so corrections and additional suggestions welcome. Expanding >> upon the source-based patching approach would be helpful, too. >> >> The second commit adds a small README.rst file in the livepatch samples >> directory pointing the reader to the doc introduced in the first commit. >> >> I didn't touch the livepatch kselftests yet as I'm still unsure about >> how to best account for IPA here. We could add the same README.rst >> disclaimer here, too, but perhaps we have a chance to do something more. >> Possibilities range from checking for renamed functions as part of their >> build, or the selftest scripts, or even adding something to the kernel >> API. I think we'll have a better idea after reviewing the compiler >> considerations doc. > > thanks to Marcos for resurrecting this. > > Joe, do you have an updated version by any chance? Some things have > changed since July 2020 so it calls for a new review. If there was an > improved version, it would be easier. If not, no problem at all. > Yea, it's been a little while :) I don't have any newer version than this one. I can rebase, apply all of the v1 suggestions, and see where it stands. LMK if you can think of any specifics that could be added. For example, CONFIG_KERNEL_IBT will be driving some changes soon, whether it be klp-convert for source-based patches or vmlinux.o binary comparison for kpatch-build. I can push a v2 with a few changes, but IIRC, last time we reviewed this, it kinda begged the question of how someone is creating the livepatch in the first place. As long as we're fine holding that thought for a while longer, this doc may still be useful by itself. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote: > > > > On Jun 1, 2024, at 03:16, Joe Lawrence wrote: > > > > Adding these attributes to livepatch sysfs would be expedient and > > probably easier for us to use, but imposes a recurring burden on us to > > maintain and test (where is the documentation and kselftest for this new > > interface?). Or, we could let the other tools handle all of that for > > us. > How this attribute imposes a recurring burden to maintain and test? > Perhaps "responsibility" is a better description. This would introduce an attribute that someone's userspace utility is relying on. It should at least have a kselftest to ensure a random patch in 2027 doesn't break it. > > Perhaps if someone already has an off-the-shelf script that is using > > ftrace to monitor livepatched code, it could be donated to > > Documentation/livepatch/? I can ask our QE folks if they have something > > like this. > > My intention to introduce this attitude to sysfs is that user who what to see > if this function is called can just need to show this function attribute in > the livepatch sysfs interface. > > User who have no experience of using ftrace will have problems to get the > calling state of the patched function. After all, ftrace is a professional > kernel tracing tools. > > Adding this attribute will be more easier for us to show if this patched > function is called. Actually, I have never try to use ftrace to trace a > patched function. Is it OK of using ftrace for a livepatched function? > If you build with CONFIG_SAMPLE_LIVEPATCH=m, you can try it out (or with one of your own livepatches): # Convenience variable $ SYSFS=/sys/kernel/debug/tracing # Install the livepatch sample demo module $ insmod samples/livepatch/livepatch-sample.ko # Verify that ftrace can filter on our functions $ grep cmdline_proc_show $SYSFS/available_filter_functions cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn off any existing tracing and filter functions $ echo 0 > $SYSFS/tracing_on $ echo > $SYSFS/set_ftrace_filter # Set up the function tracer and add the kernel's cmdline_proc_show() # and livepatch-sample's livepatch_cmdline_proc_show() $ echo function > $SYSFS/current_tracer $ echo cmdline_proc_show >> $SYSFS/set_ftrace_filter $ echo livepatch_cmdline_proc_show >> $SYSFS/set_ftrace_filter $ cat $SYSFS/set_ftrace_filter cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn on the ftracing and force execution of the original and # livepatched functions $ echo 1 > $SYSFS/tracing_on $ cat /proc/cmdline this has been live patched # Checkout out the trace file results $ cat $SYSFS/trace # tracer: function # # entries-in-buffer/entries-written: 2/2 #P:8 # #_-=> irqs-off/BH-disabled # / _=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / _-=> migrate-disable # / delay # TASK-PID CPU# | TIMESTAMP FUNCTION # | | | | | | cat-254 [002] ...2. 363.043498: cmdline_proc_show <-seq_read_iter cat-254 [002] ...1. 363.043501: livepatch_cmdline_proc_show <-seq_read_iter The kernel docs provide a lot of explanation of the complete ftracing interface. It's pretty power stuff, though you may also go the other direction and look into using the trace-cmd front end to simplify all of the sysfs manipulation. Julia Evans wrote a blog [1] a while back that provides a some more examples. [1] https://jvns.ca/blog/2017/03/19/getting-started-with-ftrace/ -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
Hi Wardenjohn, To follow up, Red Hat kpatch QE pointed me toward this test: https://gitlab.com/redhat/centos-stream/tests/kernel/kernel-tests/-/tree/main/general/kpatch/kpatch-trace?ref_type=heads which reports a few interesting things via systemd service and ftrace: - Patched functions - Traced functions - Code that was modified, but didn't run - Other code that ftrace saw, but is not listed in the sysfs directory The code looks to be built on top of the same basic ftrace commands that I sent the other day. You may be able to reuse/copy/etc bits from the scripts if they are handy. -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Fri, Sep 06, 2024 at 10:00:08AM -0700, Josh Poimboeuf wrote:
> On Fri, Sep 06, 2024 at 09:56:06AM -0400, Joe Lawrence wrote:
> > In the case of klp-diff.c, adding #include will provide the
> > memmem prototype. For both files, I needed to #define _GNU_SOURCE for
> > that prototype though.
> >
> > For the other complaint, I just set struct instruction *dest_insn = NULL
> > at the top of the for loop, but perhaps the code could be refactored to
> > clarify the situation to the compiler if you prefer not to do that.
>
> Thanks! I'll get these fixed up.
>
Also, with the workarounds mentioned above, the two you sent to Song,
and the same .config I attached in the first email, I get the following
error when trying to build the canonical /proc/cmdline example:
$ cat cmdline-string.patch
diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c
index a6f76121955f..2bcaf9ec6f78 100644
--- a/fs/proc/cmdline.c
+++ b/fs/proc/cmdline.c
@@ -7,8 +7,7 @@
static int cmdline_proc_show(struct seq_file *m, void *v)
{
- seq_puts(m, saved_command_line);
- seq_putc(m, '\n');
+ seq_printf(m, "%s kpatch=1", saved_command_line);
return 0;
}
$ ./scripts/livepatch/klp-build ./cmdline-string.patch 2>&1 | tee build2.out
- klp-build: building original kernel
vmlinux.o: warning: objtool: init_espfix_bsp+0xab: unreachable instruction
vmlinux.o: warning: objtool: init_espfix_ap+0x50: unreachable instruction
vmlinux.o: warning: objtool: syscall_init+0xca: unreachable instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: tc_wrapper_init+0x16: unreachable instruction
vmlinux.o: warning: objtool: pvh_start_xen+0x50: relocation to !ENDBR:
pvh_start_xen+0x57
- klp-build: building patched kernel
vmlinux.o: warning: objtool: init_espfix_bsp+0xab: unreachable instruction
vmlinux.o: warning: objtool: init_espfix_ap+0x50: unreachable instruction
vmlinux.o: warning: objtool: syscall_init+0xca: unreachable instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: tc_wrapper_init+0x16: unreachable instruction
vmlinux.o: warning: objtool: pvh_start_xen+0x50: relocation to !ENDBR:
pvh_start_xen+0x57
- klp-build: diffing objects
kvm.o: added: __UNIQUE_ID_nop_644
kvm.o: added: __UNIQUE_ID_nop_645
kvm.o: added: __UNIQUE_ID_nop_646
kvm.o: added: __UNIQUE_ID_nop_647
kvm.o: added: __UNIQUE_ID_nop_648
kvm.o: added: __UNIQUE_ID_nop_649
kvm.o: added: __UNIQUE_ID_nop_650
kvm.o: added: __UNIQUE_ID_nop_651
kvm.o: added: __UNIQUE_ID_nop_652
vmlinux.o: changed: cmdline_proc_show
- klp-build: building patch module
make[2]: /bin/sh: Argument list too long
make[2]: *** [scripts/Makefile.build:253:
/home/jolawren/src/linux/klp-tmp/out/livepatch.mod] Error 127
make[1]: *** [/home/jolawren/src/linux/Makefile:1943:
/home/jolawren/src/linux/klp-tmp/out] Error 2
make: *** [Makefile:240: __sub-make] Error 2
klp-build: error: module build failed
I'm guessing that this happens because of the huge dependency line in
klp-tmp/out/Kbuild for livepatch-y, it includes over 2000 object files
(that I'm pretty sure didn't change :)
I can set this up on a live machine next week if you want to investigate
further.
--
Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Fri, Sep 06, 2024 at 06:47:06PM -0700, Josh Poimboeuf wrote: > > Normally I build objtool with > > make tools/objtool > > or just > > make > > Those use the objtool Makefile without all the extra kernel flags. > > How do you normally build objtool? > Usually as part of the kernel build, for example: $ git clone \ git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git \ --branch klp-build-rfc $ cd linux && make -s defconfig && make -j$(nproc) Results in the same implicit function declaration and uninitialized variables errors. (Thanks to tools/objtool/Makefile's OBJTOOL_CFLAGS := -Werror I believe.) Re-reading my report, I thought building the two object files (check.o and klp-diff.o) individually would report their respective problems, but I see now that the invocation seems to want to build all the .o's, so disregard that build wrinkle. I almost always build objtool by a top-level `make` or `make tools/objtool`, so sorry for any confusion. -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Wed, Sep 11, 2024 at 12:39:42AM -0700, Josh Poimboeuf wrote: > On Mon, Sep 02, 2024 at 08:59:43PM -0700, Josh Poimboeuf wrote: > > Hi, > > > > Here's a new way to build livepatch modules called klp-build. > > > > I started working on it when I realized that objtool already does 99% of > > the work needed for detecting function changes. > > > > This is similar in concept to kpatch-build, but the implementation is > > much cleaner. > > > > Personally I still have reservations about the "source-based" approach > > (klp-convert and friends), including the fragility and performance > > concerns of -flive-patching. I would submit that klp-build might be > > considered the "official" way to make livepatch modules. > > > > Please try it out and let me know what you think. Based on v6.10. > > > > Also avaiable at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git > > klp-build-rfc > > Here's an updated branch with a bunch of fixes. It's still incompatible > with BTF at the moment, otherwise it should (hopefully) fix the rest of > the issues reported so far. > > While the known bugs are fixed, I haven't finished processing all the > review comments yet. Once that happens I'll post a proper v2. > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git > klp-build-v1.5 Hi Josh, I've had much better results with v1.5, thanks for collecting up those fixes in a branch. One new thing to report: depending on the optimzation of drivers/gpu/drm/vmwgfx/vmwgfx.o, objtool may throw a complaint about an unexpected relocation symbol type: $ make -j$(nproc) drivers/gpu/drm/vmwgfx/vmwgfx.o drivers/gpu/drm/vmwgfx/vmwgfx.o: error: objtool [check.c:1048]: unexpected relocation symbol type in .rela.discard.func_stack_frame_non_standard: 0 I modified check.c to print the reloc->sym->name in this case and it reports, "vmw_recv_msg". If I recreate vmwgfx.o and dump the symbol table, I notice that this is a NOTYPE symbol (probably because of vmw_recv_msg.constprop.0?) $ ld -m elf_x86_64 -z noexecstack -r -o drivers/gpu/drm/vmwgfx/vmwgfx.o @drivers/gpu/drm/vmwgfx/vmwgfx.mod $ readelf --wide --symbols drivers/gpu/drm/vmwgfx/vmwgfx.o | grep -b -e 'vmw_recv_msg' -e 'vmw_send_msg' 148334: 2198: 0010 183 FUNCLOCAL DEFAULT 1255 vmw_send_msg 151116: 2234: 0010 409 FUNCLOCAL DEFAULT 1251 vmw_recv_msg.constprop.0 180895: 2615: 0 NOTYPE GLOBAL DEFAULT UND vmw_recv_msg I don't think the config matters (I used the centos-stream-10 config) as long as the driver builds. I only saw this with a rhel-9 gcc version 11.5.0 20240719 (Red Hat 11.5.0-2) and not fedora gcc version 12.3.1 20230508 (Red Hat 12.3.1-1), which kept vmw_recv_msg w/o constprop. -- Joe
Re: [PATCH v3 0/6] livepatch: klp-convert tool - Minimal version
On Tue, Aug 27, 2024 at 02:30:45PM +0200, Lukas Hruska wrote: > Summary > --- > > This is a significantly simplified version of the original klp-convert tool. > The klp-convert code has never got a proper review and also clean ups > were not easy. The last version was v7, see > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com > > The main change is that the tool does not longer search for the > symbols which would need the livepatch specific relocation entry. > Also klp.symbols file is not longer needed. > > Instead, the needed information is appended to the symbol declaration > via a new macro KLP_RELOC_SYMBOL(). It creates symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > > The simplified klp-convert tool just transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbols with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > > Note that similar macro was needed also in the original version > to handle more symbols of the same name (sympos). > > Given the above, add klp-convert tool; integrate klp-convert tool into > kbuild; add data-structure and macros to enable users to annotate > livepatch source code; make modpost stage compatible with livepatches; > update livepatch-sample and update documentation. > > > Testing > --- > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > for both default config (with added livepatch dependencies) and a larger > SLE-15-ish config. > > > Summary of changes in this minimal version v3 > > > - klp-convert: symbol format changes (suggested by jlawrence) > - samples: fixed name of added sample in Makefile (suggested by pmladek) > - selftests: added ibt test case as an example (DON'T MERGE) > - fixed all suggested small changes in v2 > > Previous versions > - > > RFC: > https://lore.kernel.org/r/cover.1477578530.git.jpoim...@redhat.com/ > v2: > https://lore.kernel.org/r/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ > v3: > https://lore.kernel.org/r/20190410155058.9437-1-joe.lawre...@redhat.com/ > v4: > https://lore.kernel.org/r/20190509143859.9050-1-joe.lawre...@redhat.com/ > v5: > (not posted) > https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v5-devel > v6: > https://lore.kernel.org/r/20220216163940.228309-1-joe.lawre...@redhat.com/ > v7: > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com/ > v1 minimal: > https://lore.kernel.org/r/20231106162513.17556-1-lhru...@suse.cz/ > v2 minimal: > https://lore.kernel.org/r/20240516133009.20224-1-lhru...@suse.cz/ > Hi Lukas, Thanks again for posting the patchset and trying a simpler approach. I tested with latest kpatch-build tree with no ill effects -- essentially klp-convert is safe to run against .ko files that already contain klp-relocations. I would prefer more extensive selftests for various klp-relocation types (as well as symbol position), however I believe wasn't the point of the minimal version of this patchset. We can add more tests later. Anyway, now we have two RFC / patchsets supporting in-tree creation of klp-relocations (klp-convert and Josh's objtool patchset). I think we need to figure out whether one precludes the other, can they co-exist, or does that even make sense. Since LPC is right around the corner, does it make sense for folks to sync up at some point and talk pros/cons to various approaches? We don't have a microconference this year, but perhaps over lunch or beers? -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Thu, Sep 12, 2024 at 09:44:04AM -0400, Joe Lawrence wrote:
> On Wed, Sep 11, 2024 at 12:39:42AM -0700, Josh Poimboeuf wrote:
> > On Mon, Sep 02, 2024 at 08:59:43PM -0700, Josh Poimboeuf wrote:
> > > Hi,
> > >
> > > Here's a new way to build livepatch modules called klp-build.
> > >
> > > I started working on it when I realized that objtool already does 99% of
> > > the work needed for detecting function changes.
> > >
> > > This is similar in concept to kpatch-build, but the implementation is
> > > much cleaner.
> > >
> > > Personally I still have reservations about the "source-based" approach
> > > (klp-convert and friends), including the fragility and performance
> > > concerns of -flive-patching. I would submit that klp-build might be
> > > considered the "official" way to make livepatch modules.
> > >
> > > Please try it out and let me know what you think. Based on v6.10.
> > >
> > > Also avaiable at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git
> > > klp-build-rfc
> >
> > Here's an updated branch with a bunch of fixes. It's still incompatible
> > with BTF at the moment, otherwise it should (hopefully) fix the rest of
> > the issues reported so far.
> >
> > While the known bugs are fixed, I haven't finished processing all the
> > review comments yet. Once that happens I'll post a proper v2.
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git
> > klp-build-v1.5
>
> Hi Josh,
>
> I've had much better results with v1.5, thanks for collecting up those
> fixes in a branch.
>
Today's experiment used the centos-stream-10's kernel config with
CONFIG_MODULE_ALLOW_BTF_MISMATCH=y and cs-10's gcc (GCC) 14.2.1 20240801
(Red Hat 14.2.1-1).
First, more gcc nits (running top-level `make`):
check.c: In function ‘decode_instructions’:
check.c:410:54: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier
argument and not in the later argument [-Werror=calloc-transposed-args]
410 | insns = calloc(sizeof(*insn),
INSN_CHUNK_SIZE);
| ^
check.c:410:54: note: earlier argument should specify number of elements,
later size of each element
check.c: In function ‘init_pv_ops’:
check.c:551:38: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier
argument and not in the later argument [-Werror=calloc-transposed-args]
551 | file->pv_ops = calloc(sizeof(struct pv_state), nr);
| ^~
check.c:551:38: note: earlier argument should specify number of elements,
later size of each element
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 63c2d6c06..c6f192859 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -407,7 +407,7 @@ static void decode_instructions(struct objtool_file *file)
for (offset = 0; offset < sec_size(sec); offset += insn->len) {
if (!insns || idx == INSN_CHUNK_MAX) {
- insns = calloc(sizeof(*insn), INSN_CHUNK_SIZE);
+ insns = calloc(INSN_CHUNK_SIZE, sizeof(*insn));
ERROR_ON(!insns, "calloc");
idx = 0;
@@ -548,7 +548,7 @@ static void init_pv_ops(struct objtool_file *file)
return;
nr = sym->len / sizeof(unsigned long);
- file->pv_ops = calloc(sizeof(struct pv_state), nr);
+ file->pv_ops = calloc(nr, sizeof(struct pv_state));
ERROR_ON(!file->pv_ops, "calloc");
for (idx = 0; idx < nr; idx++)
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
and now a happy build of objtool.
The top-level `make` moves onto building all the kernel objects, but
then objtool vmlinux.o crashes:
$ gdb --args ./tools/objtool/objtool --sym-checksum --hacks=jump_label
--hacks=noinstr --hacks=skylake --ibt --orc --retpoline --rethunk --static-call
--uaccess --prefix=16 --link vmlinux.o
Program received signal SIGSEGV, Segmentation fault.
ignore_unreachable_insn (file=0x435ea0 , insn=0x1cd928c0) at
check.c:3980
3980if (prev_insn->dead_end &&
(gdb) bt
#0 ignore_unreachable_insn (file=0x435ea0 , insn=0x1cd928c0) at
check.c:3980
#1 validate_reachable_instructions (file=0x435ea0 ) at check.c:4452
#2 check (file=file@entry=0x435ea0 ) at check.c:4610
#3 0x00
Re: [SCSI RFC] mpt2sas: wait_for_completion_timeout timeout not reported
On 12/29/2014 12:25 PM, Nicholas Mc Guire wrote:
> wait_for_completion_timeout reaching timeout was being ignored,
> this probably also should fail if timeout condition occurs ?
>
> this was only compile tested with
> x86_64_defconfig + CONFIG_SCSI_LOWLEVEL=y + CONFIG_SCSI_MPT2SAS=m
>
> patch is against linux-next 3.19.0-rc1 -next-20141226
>
> Signed-off-by: Nicholas Mc Guire
> ---
> drivers/scsi/mpt2sas/mpt2sas_config.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_config.c
> b/drivers/scsi/mpt2sas/mpt2sas_config.c
> index c72a2ff..02dc2d8 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_config.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_config.c
> @@ -391,7 +391,7 @@ _config_request(struct MPT2SAS_ADAPTER *ioc,
> Mpi2ConfigRequest_t
> mpt2sas_base_put_smid_default(ioc, smid);
> timeleft = wait_for_completion_timeout(&ioc->config_cmds.done,
> timeout*HZ);
> - if (!(ioc->config_cmds.status & MPT2_CMD_COMPLETE)) {
> + if (timeleft == 0 || !(ioc->config_cmds.status & MPT2_CMD_COMPLETE)) {
> printk(MPT2SAS_ERR_FMT "%s: timeout\n",
> ioc->name, __func__);
> _debug_dump_mf(mpi_request,
>
Hi Nicholas,
This would look like the correct thing to do here, for it eliminates a
small window where we timeout when mpt2sas_config_done had set
MPT2_CMD_COMPLETE and MPT3_CMD_REPLY_VALID, but has not finished copying
the reply data into config_cmds.reply.
It looks like the same patch should be applied to the mpt3sas driver as
well.
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [SCIS] mpt3sas: wait_for_completion_timeout timeout not reported
On 12/30/2014 11:19 AM, Nicholas Mc Guire wrote:
> wait_for_completion_timeout reaching timeout was being ignored,
> this also should fail if timeout condition occurs.
>
> Thanks to Joe Lawrence for confirmation.
How about this instead:
Acked-by: Joe Lawrence
You still probably want a review from the Avago folks though.
BTW, don't worry about the "[SCSI]" subject prefix to the commit,
"[PATCH]" is fine. I believe the former is a convention that the
maintainer applies to patches as he collects them to indicate that they
originated via the SCSI tree.
Thanks,
-- Joe
> this was only compile tested with
> x86_64_defconfig + CONFIG_SCSI_LOWLEVEL=y + CONFIG_SCSI_MPT3SAS=m
>
> patch is against linux-next 3.19.0-rc1 -next-20141226
> Signed-off-by: Nicholas Mc Guire
> ---
> drivers/scsi/mpt3sas/mpt3sas_config.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c
> b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index 4472c2a..04ff21b 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -393,7 +393,7 @@ _config_request(struct MPT3SAS_ADAPTER *ioc,
> Mpi2ConfigRequest_t
> mpt3sas_base_put_smid_default(ioc, smid);
> timeleft = wait_for_completion_timeout(&ioc->config_cmds.done,
> timeout*HZ);
> - if (!(ioc->config_cmds.status & MPT3_CMD_COMPLETE)) {
> + if (timeleft == 0 || !(ioc->config_cmds.status & MPT3_CMD_COMPLETE)) {
> pr_err(MPT3SAS_FMT "%s: timeout\n",
> ioc->name, __func__);
> _debug_dump_mf(mpi_request,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 17/20] [SCSI] mpt3sas: Use alloc_ordered_workqueue() API instead of create_singlethread_workqueue() API
On 06/12/2015 05:42 AM, Sreekanth Reddy wrote: ... > +#if defined(alloc_ordered_workqueue) > + ioc->firmware_event_thread = alloc_ordered_workqueue( > + ioc->firmware_event_name, WQ_MEM_RECLAIM); > +#else > + ioc->firmware_event_thread = create_singlethread_workqueue( > ioc->firmware_event_name); > +#endif Hi Sreekanth, I think the upstream version of this code can safely assume alloc_ordered_workqueue is defined, no? Regards, -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 17/20 v1] [SCSI] mpt3sas: Use alloc_ordered_workqueue() API instead of create_singlethread_workqueue() API
On 06/16/2015 01:37 AM, Sreekanth Reddy wrote:
> Created a thread using alloc_ordered_workqueue() API in order to process
> the works from firmware Work-queue sequentially instead of
> create_singlethread_workqueue() API.
>
> Changes in v1:
> No need to check for backport compatibility in the upstream kernel.
> so removing the else section where driver use
> create_singlethread_workqueue() API if alloc_ordered_workqueue() API is
> not defined, This else section is not required since in the latest upstream
> kernel this alloc_ordered_workqueue() API is always defined.
>
> Signed-off-by: Sreekanth Reddy
> ---
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index b848458..7e5926c 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -8085,8 +8085,8 @@ _scsih_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> /* event thread */
> snprintf(ioc->firmware_event_name, sizeof(ioc->firmware_event_name),
> "fw_event%d", ioc->id);
> - ioc->firmware_event_thread = create_singlethread_workqueue(
> - ioc->firmware_event_name);
> + ioc->firmware_event_thread = alloc_ordered_workqueue(
> + ioc->firmware_event_name, WQ_MEM_RECLAIM);
> if (!ioc->firmware_event_thread) {
> pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
> ioc->name, __FILE__, __LINE__, __func__);
>
Hi Sreekanth,
Is this change still needed after e09c2c2954684 workqueue: apply
__WQ_ORDERED to create_singlethread_workqueue() ? (3.17+)
In upstream, this change looks cosmetic (unless Tejun has a preference
for one over the other), but maybe converting to alloc_ordered_workqueue
keeps your in house version in closer sync?
Thanks,
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 17/20 v1] [SCSI] mpt3sas: Use alloc_ordered_workqueue() API instead of create_singlethread_workqueue() API
On 06/18/2015 09:06 AM, Sreekanth Reddy wrote:
> On Thu, Jun 18, 2015 at 5:40 PM, Joe Lawrence
> wrote:
>> On 06/16/2015 01:37 AM, Sreekanth Reddy wrote:
>>> Created a thread using alloc_ordered_workqueue() API in order to process
>>> the works from firmware Work-queue sequentially instead of
>>> create_singlethread_workqueue() API.
>>>
>>> Changes in v1:
>>> No need to check for backport compatibility in the upstream kernel.
>>> so removing the else section where driver use
>>> create_singlethread_workqueue() API if alloc_ordered_workqueue() API is
>>> not defined, This else section is not required since in the latest upstream
>>> kernel this alloc_ordered_workqueue() API is always defined.
>>>
>>> Signed-off-by: Sreekanth Reddy
>>> ---
>>> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> index b848458..7e5926c 100644
>>> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> @@ -8085,8 +8085,8 @@ _scsih_probe(struct pci_dev *pdev, const struct
>>> pci_device_id *id)
>>> /* event thread */
>>> snprintf(ioc->firmware_event_name, sizeof(ioc->firmware_event_name),
>>> "fw_event%d", ioc->id);
>>> - ioc->firmware_event_thread = create_singlethread_workqueue(
>>> - ioc->firmware_event_name);
>>> + ioc->firmware_event_thread = alloc_ordered_workqueue(
>>> + ioc->firmware_event_name, WQ_MEM_RECLAIM);
>>> if (!ioc->firmware_event_thread) {
>>> pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
>>> ioc->name, __FILE__, __LINE__, __func__);
>>>
>>
>> Hi Sreekanth,
>>
>> Is this change still needed after e09c2c2954684 workqueue: apply
>> __WQ_ORDERED to create_singlethread_workqueue() ? (3.17+)
>
> I won't say that it is compulsory required, but I feel it is better if
> these changes are included. since initially we thought that thread
> created by using create_singlethread_workqueue() will process the
> works sequentially but in-between it has broken and then it is fixed
> by Tejun. So I thought it is better to directly use the
> alloc_ordered_workqueue() as create_singlethead_workqueue() API also
> invoked the same API.
Ok, I was just wondering if maybe create_singlethread_workqueue was
fixed after this patch was initially written. Since it's effectively
the same...
Reviewed-by: Joe Lawrence
Regards,
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add livepatch kselftests
Hi all, This patch was motivated by the increasing corner cases in the livepatch code. These tests don't cover anywhere near all of them, but were mostly already written up as demonstrations / documentation of the livepatch callback functionality. Converting them into a test rig didn't take long. So the testing methodology is a little hokey as it relies on dmesg order and filtering. That said, it's already been used to verify livepatch callback for "livepatch: Atomic replace feature" patchset. Any suggestions for better testing would be welcome. I didn't get to the shadow variable testing script yet. I figured that I'd share the current state before I got too far along in writing tests. Suggestions for additional tests and test categories welcome as well. To try out the tests, configure with: CONFIG_TEST_LIVEPATCH=m Build kernel, modules, install, etc. then reboot and kick off: % make -C tools/testing/selftests TARGETS=livepatch run_tests The tests expect to on top of v10+ of the "livepatch: Atomic replace feature" series and take ~2.5 minutes to complete. rfc changes: - SPDX-License-Identifiers - Moved livepatch test modules into lib/livepatch - Renamed livepatch.sh (filename suffix) - Reduced between-test delay time - Split off common functions.sh file - Split into separate livepatch, callbacks, and shadow-vars scrips - Gave the tests short descriptions instead of TEST1, TEST2, etc. Joe Lawrence (1): selftests/livepatch: introduce tests lib/Kconfig.debug | 12 + lib/Makefile | 2 + lib/livepatch/Makefile | 18 + lib/livepatch/test_klp_atomic_replace.c| 69 +++ lib/livepatch/test_klp_callbacks_busy.c| 43 ++ lib/livepatch/test_klp_callbacks_demo.c| 132 ++ lib/livepatch/test_klp_callbacks_demo2.c | 104 lib/livepatch/test_klp_callbacks_mod.c | 24 + lib/livepatch/test_klp_livepatch.c | 62 +++ tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile | 8 + tools/testing/selftests/livepatch/config | 1 + tools/testing/selftests/livepatch/functions.sh | 202 .../testing/selftests/livepatch/test-callbacks.sh | 526 + .../testing/selftests/livepatch/test-livepatch.sh | 177 +++ .../selftests/livepatch/test-shadow-vars.sh| 13 + 16 files changed, 1394 insertions(+) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh -- 1.8.3.1
[PATCH] selftests/livepatch: introduce tests
Add a few livepatch modules and simple target modules that the included
regression suite can run tests against.
Signed-off-by: Joe Lawrence
---
lib/Kconfig.debug | 12 +
lib/Makefile | 2 +
lib/livepatch/Makefile | 18 +
lib/livepatch/test_klp_atomic_replace.c| 69 +++
lib/livepatch/test_klp_callbacks_busy.c| 43 ++
lib/livepatch/test_klp_callbacks_demo.c| 132 ++
lib/livepatch/test_klp_callbacks_demo2.c | 104
lib/livepatch/test_klp_callbacks_mod.c | 24 +
lib/livepatch/test_klp_livepatch.c | 62 +++
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/livepatch/Makefile | 8 +
tools/testing/selftests/livepatch/config | 1 +
tools/testing/selftests/livepatch/functions.sh | 202
.../testing/selftests/livepatch/test-callbacks.sh | 526 +
.../testing/selftests/livepatch/test-livepatch.sh | 177 +++
.../selftests/livepatch/test-shadow-vars.sh| 13 +
16 files changed, 1394 insertions(+)
create mode 100644 lib/livepatch/Makefile
create mode 100644 lib/livepatch/test_klp_atomic_replace.c
create mode 100644 lib/livepatch/test_klp_callbacks_busy.c
create mode 100644 lib/livepatch/test_klp_callbacks_demo.c
create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c
create mode 100644 lib/livepatch/test_klp_callbacks_mod.c
create mode 100644 lib/livepatch/test_klp_livepatch.c
create mode 100644 tools/testing/selftests/livepatch/Makefile
create mode 100644 tools/testing/selftests/livepatch/config
create mode 100644 tools/testing/selftests/livepatch/functions.sh
create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh
create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh
create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64155e310a9f..e4a0e81542ff 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1932,6 +1932,18 @@ config TEST_DEBUG_VIRTUAL
If unsure, say N.
+config TEST_LIVEPATCH
+ tristate "Test livepatching"
+ default n
+ depends on LIVEPATCH
+ depends on m
+ help
+ Test various kernel livepatching features for correctness.
+ The tests will load test modules that will be livepatched
+ in various scenarios.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index a90d4fcd748f..98a38441afb0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -67,6 +67,8 @@ obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
+obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
+
ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
CFLAGS_kobject_uevent.o += -DDEBUG
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
new file mode 100644
index ..3c588564e518
--- /dev/null
+++ b/lib/livepatch/Makefile
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for livepatch test code.
+
+obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
+ test_klp_callbacks_demo.o \
+ test_klp_callbacks_demo2.o \
+ test_klp_callbacks_busy.o \
+ test_klp_callbacks_mod.o \
+ test_klp_livepatch.o
+
+# Livepatch modules require CC_FLAGS_FTRACE to hook functions
+CFLAGS_test_klp_atomic_replace.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_demo.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_demo2.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_callbacks_mod.o+= $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_livepatch.o+= $(CC_FLAGS_FTRACE)
diff --git a/lib/livepatch/test_klp_atomic_replace.c
b/lib/livepatch/test_klp_atomic_replace.c
new file mode 100644
index ..481941050e60
--- /dev/null
+++ b/lib/livepatch/test_klp_atomic_replace.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+
+static int replace;
+module_param(replace, int, 0644);
+MODULE_PARM_DESC(replace, "replace (default=0)");
+
+#include
+static int livepatch_meminfo_proc_show(struct seq_file *m, void *v)
+{
+ seq_printf(m, "%s: %s\n", THIS_MODULE->name,
+ "this has been live patched");
+ return 0;
+}
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "meminfo_proc_show",
+ .new_func = livepatch_meminfo
[PATCH v4 0/1] Add livepatch kselftests
This round incorporates feedback from SUSE folks, Miroslav, Petr and Libor. Thanks for the reviews and feedback! Like the previous version, this applies on top of Petr's shadow variable changes and the atomic replace patchset. -- Joe changes from v3: - add MAINTAINERS entry for tools/testing/selftests/livepatch/ - make more test_klp_shadow_vars.c functions static (Miroslav) - use git format-patch --base to generate base-commit and prerequisite patch info (kbuild test robot) - tweak TEST_LIVEPATCH help text (Petr) - add note in callbacks.txt pointing to sample/test examples (Petr) - add a kmsg log() function (Libor) - various whitespace and comment cleanups (Libor) - add "$(dirname $0)/" directory prefix to functions.sh sourcing (Libor) - add loop_until() function instead of redundant inline retry/loops (Libor) - wait_for_transition() looks for any transition (Libor) changes from v2: - fix module_exit(test_klp_shadow_vars_exit) in test_klp_shadow_vars.c - silence kbuild test robot's "XXX can be static" and "Using plain integer as NULL pointer" complaints - re-run tests with CONFIG_LOCKDEP=y and CONFIG_PROVE_LOCKING=y - use GFP_ATOMIC in test_klp_shadow_vars.c constructor code changes from v1: - Only add $(CC_FLAGS_FTRACE) for target modules - Remove between test delay - Reduce RETRY_INTERVAL to .1 sec - Reduce test_callback_mod's busymod_work_func delay from 60 to 10 sec - s/PASS/ok and s/FAIL/not ok for test output - Move test descriptions from Documentation/livepatch/callbacks.txt into tools/testing/selftests/livepatch/test-callbacks.sh - Add a shadow variable test script and module - Add a short tools/testing/selftests/livepatch/README - to += linux-kselft...@vger.kernel.org - cc += Libor, Nicolai, Artem changes from rfc: - SPDX-License-Identifiers - Moved livepatch test modules into lib/livepatch - Renamed livepatch.sh (filename suffix) - Reduced between-test delay time - Split off common functions.sh file - Split into separate livepatch, callbacks, and shadow-vars scrips - Gave the tests short descriptions instead of TEST1, TEST2, etc. Joe Lawrence (1): selftests/livepatch: introduce tests Documentation/livepatch/callbacks.txt | 489 +- MAINTAINERS | 1 + lib/Kconfig.debug | 21 + lib/Makefile | 2 + lib/livepatch/Makefile| 15 + lib/livepatch/test_klp_atomic_replace.c | 69 ++ lib/livepatch/test_klp_callbacks_busy.c | 43 ++ lib/livepatch/test_klp_callbacks_demo.c | 132 lib/livepatch/test_klp_callbacks_demo2.c | 104 +++ lib/livepatch/test_klp_callbacks_mod.c| 24 + lib/livepatch/test_klp_livepatch.c| 62 ++ lib/livepatch/test_klp_shadow_vars.c | 236 +++ tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile| 8 + tools/testing/selftests/livepatch/README | 43 ++ tools/testing/selftests/livepatch/config | 1 + .../testing/selftests/livepatch/functions.sh | 164 + .../selftests/livepatch/test-callbacks.sh | 607 ++ .../selftests/livepatch/test-livepatch.sh | 173 + .../selftests/livepatch/test-shadow-vars.sh | 60 ++ 20 files changed, 1771 insertions(+), 484 deletions(-) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 lib/livepatch/test_klp_shadow_vars.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/README create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh base-commit: 0adb32858b0bddf4ada5f364a84ed60b196dbcda prerequisite-patch-id: 5ed747c1a89a5dc4bba08186e21f927d7f3bf049 prerequisite-patch-id: e9800288b71a9f339ea066e58d9ef70dece67083 prerequisite-patch-id: 415f2e190b1b50142c78f2940c7b8dd39b5321a0 prerequisite-patch-id: d229d9cf08af087e0a758d9df1da467103c2c200 prerequisite-patch-id: b8c7ef99b13c6b321cba5e8919ed0b3e29f213e9 prerequisite-patch-id: 4e10c0d08f151b18310fe0b1e5013d62db94cfeb prerequisite-patch-id: 33046b190c114d202f3a52e0e274dbb2b1907a4c prerequisite-patch-id: 6978944a725756317dd4e005d479b6101784aaf0 prerequisite-patch-id: cce9d3c7e1ae8887f387ca9e072552dc63479749 prerequisite-patch-id: c44ccc5dd7
[PATCH v4 1/1] selftests/livepatch: introduce tests
Add a few livepatch modules and simple target modules that the included regression suite can run tests against: - basic livepatching (multiple patches, atomic replace) - pre/post (un)patch callbacks - shadow variable API Signed-off-by: Joe Lawrence --- Documentation/livepatch/callbacks.txt | 489 +- MAINTAINERS | 1 + lib/Kconfig.debug | 21 + lib/Makefile | 2 + lib/livepatch/Makefile| 15 + lib/livepatch/test_klp_atomic_replace.c | 69 ++ lib/livepatch/test_klp_callbacks_busy.c | 43 ++ lib/livepatch/test_klp_callbacks_demo.c | 132 lib/livepatch/test_klp_callbacks_demo2.c | 104 +++ lib/livepatch/test_klp_callbacks_mod.c| 24 + lib/livepatch/test_klp_livepatch.c| 62 ++ lib/livepatch/test_klp_shadow_vars.c | 236 +++ tools/testing/selftests/Makefile | 1 + tools/testing/selftests/livepatch/Makefile| 8 + tools/testing/selftests/livepatch/README | 43 ++ tools/testing/selftests/livepatch/config | 1 + .../testing/selftests/livepatch/functions.sh | 164 + .../selftests/livepatch/test-callbacks.sh | 607 ++ .../selftests/livepatch/test-livepatch.sh | 173 + .../selftests/livepatch/test-shadow-vars.sh | 60 ++ 20 files changed, 1771 insertions(+), 484 deletions(-) create mode 100644 lib/livepatch/Makefile create mode 100644 lib/livepatch/test_klp_atomic_replace.c create mode 100644 lib/livepatch/test_klp_callbacks_busy.c create mode 100644 lib/livepatch/test_klp_callbacks_demo.c create mode 100644 lib/livepatch/test_klp_callbacks_demo2.c create mode 100644 lib/livepatch/test_klp_callbacks_mod.c create mode 100644 lib/livepatch/test_klp_livepatch.c create mode 100644 lib/livepatch/test_klp_shadow_vars.c create mode 100644 tools/testing/selftests/livepatch/Makefile create mode 100644 tools/testing/selftests/livepatch/README create mode 100644 tools/testing/selftests/livepatch/config create mode 100644 tools/testing/selftests/livepatch/functions.sh create mode 100755 tools/testing/selftests/livepatch/test-callbacks.sh create mode 100755 tools/testing/selftests/livepatch/test-livepatch.sh create mode 100755 tools/testing/selftests/livepatch/test-shadow-vars.sh diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt index c9776f48e458..182e31d4abce 100644 --- a/Documentation/livepatch/callbacks.txt +++ b/Documentation/livepatch/callbacks.txt @@ -118,488 +118,9 @@ similar change to their hw_features value. (Client functions of the value may need to be updated accordingly.) -Test cases -== - -What follows is not an exhaustive test suite of every possible livepatch -pre/post-(un)patch combination, but a selection that demonstrates a few -important concepts. Each test case uses the kernel modules located in -the samples/livepatch/ and assumes that no livepatches are loaded at the -beginning of the test. - - -Test 1 --- - -Test a combination of loading a kernel module and a livepatch that -patches a function in the first module. (Un)load the target module -before the livepatch module: - -- load target module -- load livepatch -- disable livepatch -- unload target module -- unload livepatch - -First load a target module: - - % insmod samples/livepatch/livepatch-callbacks-mod.ko - [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init - -On livepatch enable, before the livepatch transition starts, pre-patch -callbacks are executed for vmlinux and livepatch_callbacks_mod (those -klp_objects currently loaded). After klp_objects are patched according -to the klp_patch, their post-patch callbacks run and the transition -completes: - - % insmod samples/livepatch/livepatch-callbacks-demo.ko - [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo' - [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing patching transition - [ 36.504238] livepatch_callbacks_demo: pre_patch_callback: vmlinux - [ 36.504721] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 36.505849] livepatch: 'livepatch_callbacks_demo': starting patching transition - [ 37.727133] livepatch: 'livepatch_callbacks_demo': completing patching transition - [ 37.727232] livepatch_callbacks_demo: post_patch_callback: vmlinux - [ 37.727860] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state - [ 37.728792] livepatch: 'livepatch_callbacks_demo': patching complete - -Similarly, on livepatch disable, pre-patch callbacks run before the -unpatching transition starts. klp_objects are reverted, post-patch -callbacks execute and the transition completes: - - % echo 0
Re: [PATCH v4 0/1] Add livepatch kselftests
>On 04/25/2018 02:28 PM, Joe Lawrence wrote: > [ ... snip ... ] > > base-commit: 0adb32858b0bddf4ada5f364a84ed60b196dbcda > prerequisite-patch-id: 5ed747c1a89a5dc4bba08186e21f927d7f3bf049 > prerequisite-patch-id: e9800288b71a9f339ea066e58d9ef70dece67083 > prerequisite-patch-id: 415f2e190b1b50142c78f2940c7b8dd39b5321a0 > prerequisite-patch-id: d229d9cf08af087e0a758d9df1da467103c2c200 > prerequisite-patch-id: b8c7ef99b13c6b321cba5e8919ed0b3e29f213e9 > prerequisite-patch-id: 4e10c0d08f151b18310fe0b1e5013d62db94cfeb > prerequisite-patch-id: 33046b190c114d202f3a52e0e274dbb2b1907a4c > prerequisite-patch-id: 6978944a725756317dd4e005d479b6101784aaf0 > prerequisite-patch-id: cce9d3c7e1ae8887f387ca9e072552dc63479749 > prerequisite-patch-id: c44ccc5dd7b1be6fe2b1f32ca6abde1da73fae79 > Hi kbuild test robot folks, I attempted to use the --base option with git format-patch as suggested by Philip, but the bot still sent me mail (addressed only to myself and cc'd kbuild-...@01.org) about build test ERRORs against the wrong base: > [auto build test ERROR on v4.16] > [also build test ERROR on next-20180424] > [cannot apply to linus/master jikos-livepatching/for-next] I'm assuming operator error :( Here's a summary of my workflow: * Save an .mbox of the entire base patchset, as posted to the live-patching list: https://lkml.org/lkml/2018/3/23/665 * Create a "base" branch and apply the mbox: % git checkout -b test_base v4.16 Switched to a new branch 'test_base' % git am /tmp/pm.mbox Applying: livepatch: Use lists to manage patches, objects and functions Applying: livepatch: Free only structures with initialized kobject Applying: livepatch: Add atomic replace Applying: livepatch: Add an extra flag to distinguish registered patches Applying: livepatch: Remove replaced patches from the stack Applying: livepatch: Remove Nop structures when unused Applying: livepatch: Allow to replace even disabled patches Applying: livepatch: Atomic replace and cumulative patches documentation * Create a new dev branch from the base, make a trivial change and commit: % git checkout -b test_branch test_base % sed -i 's/^EXTRAVERSION =/EXTRAVERSION = .test/' Makefile % git commit Makefile -m 'test commit' * Create .patch files with --base: % git format-patch --base=v4.16 -1 --cover-letter % grep -e '^base-commit' -e 'prereq' -cover-letter.patch base-commit: 0adb32858b0bddf4ada5f364a84ed60b196dbcda prerequisite-patch-id: 5ed747c1a89a5dc4bba08186e21f927d7f3bf049 prerequisite-patch-id: e9800288b71a9f339ea066e58d9ef70dece67083 prerequisite-patch-id: 415f2e190b1b50142c78f2940c7b8dd39b5321a0 prerequisite-patch-id: d229d9cf08af087e0a758d9df1da467103c2c200 prerequisite-patch-id: b8c7ef99b13c6b321cba5e8919ed0b3e29f213e9 prerequisite-patch-id: 4e10c0d08f151b18310fe0b1e5013d62db94cfeb prerequisite-patch-id: 33046b190c114d202f3a52e0e274dbb2b1907a4c prerequisite-patch-id: 6978944a725756317dd4e005d479b6101784aaf0 I notice that these patch-ids are only added to the cover-letter... do I need to force them each individual patch as well? /confused Thanks, -- Joe
Re: [PATCH] x86/stacktrace: export save_stack_trace_tsk_reliable
On 2/27/19 5:25 PM, Joe Lawrence wrote: On 2/27/19 4:31 PM, Thomas Gleixner wrote: On Wed, 27 Feb 2019, Joe Lawrence wrote: The ppc64le implementation of save_stack_trace_tsk_reliable() is exported, so do the same with x86. And what's the in tree module user of this? I can't find one and just because PPC has an export with no user is not a convincing argument to add another one. The proper solution is to remove the unused PPC export. Good point. For that matter, I do see in-tree modules making use of save_stack_trace, but who is calling save_stack_trace_tsk (exported by most arches) and save_stack_trace_regs (exported by openrisc, powerpc, s390)? Well, at least for save_stack_trace_tsk there is the out-of-tree kpatch core module[1]. (Kpatch drops that call if the kernel provides livepatch functionality.) [1] https://github.com/dynup/kpatch/blob/master/kmod/core/core.c#L275 -- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/16/19 1:24 AM, Balbir Singh wrote: Could we get some details with examples or a sample, sorry I might be dense and missing simple information. The problem being solved is not clear to me from the changelog. Hi Balbir, I see that Miroslav has already pointed to documentation, samples and self-tests for the patchset, but here is a summary in my own words: kpatch-build constructs livepatch kernel modules by comparing a reference build with a patched build, combing through ELF object sections and extracting new and changed sections that include patched code. An alternative approach is "source-based", in which a livepatch kernel module is built (mostly entirely) using the kernel's build infrastructure. Sources for a livepatch are gathered ahead of time and then built like an ordinary kernel module. In either approach, there lies the problem of symbol visibility: how can a livepatch resolve symbols that a kernel module ordinarily can't. For example, file local or simply unexported symbols in across the kernel and other modules. Enter the concept of "livepatch symbols" described in module-elf-format.txt. kpatch-build already creates such "livepatch symbols" (see its create_klp_relasecs_and_syms()) and the livepatching core kernel code already knows how resolve such symbols at klp_object patch time (see klp_write_object_relocations()). The klp-convert tool and this supporting patchset would empower source-based-constructed livepatch modules to do the same. -- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/10/19 11:50 AM, Joe Lawrence wrote: Hi folks, This is the third installment of the klp-convert tool for generating and processing livepatch symbols for livepatch module builds. For those following along at home, archive links to previous versions: RFC: https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/ v2: https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ (Note that I don't see v2 archived at lore, but that is a link to the most recent subthread that lore did catch.) Livepatches may use symbols which are not contained in its own scope, and, because of that, may end up compiled with relocations that will only be resolved during module load. Yet, when the referenced symbols are not exported, solving this relocation requires information on the object that holds the symbol (either vmlinux or modules) and its position inside the object, as an object may contain multiple symbols with the same name. Providing such information must be done accordingly to what is specified in Documentation/livepatch/module-elf-format.txt. Hi Miroslav, I noticed that some binutils programs like gdb, objdump, etc. don't like the .ko kernel objects that we're generating from this patchset, specifically those with the additional '.klp.rela...text' livepatch symbol relocation sections. For reference, I opened a new bugzilla with more details here: https://sourceware.org/bugzilla/show_bug.cgi?id=24456 And was about to ping the binutils mailing list about the assertion that is tripping in bfd/elf.c. The thought occurred to me that you guys might already be carrying a patch to workaround this issue? -- Joe
Re: [PATCH v3 2/9] kbuild: Support for Symbols.list creation
On 4/11/19 3:04 PM, Miroslav Benes wrote:
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 2472ce39a18d..8b9b42a258ad 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1,3 +1,4 @@
+LIVEPATCH_livepatch-sample := y
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
We discussed this and I think there has been no conclusion yet. Shouldn't
we do the same (add LIVEPATCH_) for the other samples here as well?
In v2 review, I mentioned moving this hunk to ("modpost: Add modinfo
flag to livepatch modules") along with the same change to the other
sample modules. I didn't apply that for v3, but can do so for the next
spin if it makes the commits easier to review.
-- Joe
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> TODO
>
>
> There are a few outstanding items that I came across while reviewing v2,
> but as changes started accumulating, it made sense to defer those to a
> later version. I'll reply to this thread individual topics to start
> discussion sub-threads for those.
Multiple object files
=
For v3, we tweaked the build scripts so that we could successfully build
a klp-converted livepatch module from multiple object files.
One interesting use-case is supporting two livepatch symbols of the same
name, but different object/position values.
An example target kernel module might be layed out like this:
test_klp_convert_mod.ko << target module is comprised of
two object files, each defining
test_klp_convert_mod_a.o their own local get_homonym_string()
get_homonym_string() function and homonym_string[]
homonym_string[] character arrays.
test_klp_convert_mod_b.o
get_homonym_string()
homonym_string[]
A look at interesting parts of the the module's symbol table:
% eu-readelf --symbols lib/livepatch/test_klp_convert_mod.ko | \
grep -e 'homonym' -e test_klp_convert_mod_
29: 0 FILELOCAL DEFAULT ABS
test_klp_convert_mod_a.c
32: 0010 8 FUNCLOCAL DEFAULT3
get_homonym_string
33: 17 OBJECT LOCAL DEFAULT5 homonym_string
37: 0 FILELOCAL DEFAULT ABS
test_klp_convert_mod_b.c
38: 0020 8 FUNCLOCAL DEFAULT3
get_homonym_string
39: 17 OBJECT LOCAL DEFAULT 11 homonym_string
suggests that any livepatch module that wishes to resolve to
test_klp_convert_mod_a.c :: get_homonym_string() should include an
annotation like:
file_a.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_a[] = {
KLP_SYMPOS(get_homonym_string, 1),
};
and to resolve test_klp_convert_mod_b.c :: get_homonym_string():
file_b.c:
KLP_MODULE_RELOC(test_klp_convert_mod) relocs_b[] = {
KLP_SYMPOS(get_homonym_string, 2),
};
Unfortunately klp-convert v3 will fail to build such livepatch module,
regardless of sympos values:
klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,2 vs.
vmlinux.state_show,1.
klp-convert: Unable to load user-provided sympos
make[1]: *** [scripts/Makefile.modpost:148:
lib/livepatch/test_klp_convert_multi_objs.ko] Error 255
This abort message can be traced to sympos_sanity_check() as it verifies
that there no duplicate symbol names found in its list of user specified
symbols (ie, those needing to be converted).
Common sympos
-
New test case and related fixup can be found here:
https://github.com/joe-lawrence/linux/commits/klp-convert-v4-multiple-objs
To better debug this issue, I added another selftest that demonstrates
this configuration in isolation. "show_state" is a popular kernel
function name (my VM reported 5 instances in kallsyms) so I chose that
symbol name.
Initially I specified the *same* symbol position (1) in both .c file
KLP_SYMPOS annotations. At the very least, we can trivially patch
klp-convert v3 to support annotations for the the same symbol value across object files.
Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification: allow for duplicate
instances. Now it will only complain when a supplied symbol references
the same but a different .
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..713835dfc9ec 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
}
}
-/* Checks if two or more elements in usr_symbols have the same name */
+/*
+ * Checks if two or more elements in usr_symbols have the same
+ * object and name, but different symbol position
+ */
static bool sympos_sanity_check(void)
{
bool sane = true;
@@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
list_for_each_entry(sp, &usr_symbols, list) {
aux = list_next_entry(sp, list);
list_for_each_entry_from(aux, &usr_symbols, list) {
- if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
+ if (sp->pos != aux->pos &&
+ strcmp(sp->object_name, aux->object_name) == 0 &&
+ strcmp(sp->symbol_name, aux->symbol_name) == 0)
WARN("Conflicting KLP_SYMPOS definition:
%s.%s,%d vs. %s.%s,%d.",
sp->
Re: [PATCH v3 3/9] livepatch: Add klp-convert tool
On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
>
> [ ... snip ... ]
>
> +static bool convert_rela(struct section *oldsec, struct rela *r,
> + struct sympos *sp, struct elf *klp_elf)
> +{
> + struct section *sec;
> + struct rela *r1, *r2;
> +
> + sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
> + if (!sec) {
> + WARN("Can't create or access klp.rela section (%s.%s)\n",
> + sp->object_name, sp->symbol_name);
> + return false;
> + }
> +
> + if (!convert_klp_symbol(r->sym, sp)) {
> + WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
> + r->sym->name);
> + return false;
> + }
> +
> + /* Move the converted rela to klp rela section */
> + list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
> + if (r1->sym->name == r->sym->name) {
> + list_del(&r1->list);
> + list_add(&r1->list, &sec->relas);
> + }
> + }
> + return true;
> +}
This one took a while to find and debug, but I believe that
convert_rela()'s list removal is not as safe as it thinks it is.
Start with its calling context from main() below:
> + list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
> + if (!is_rela_section(sec))
> + continue;
> +
> + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> + if (!must_convert(rela->sym))
> + continue;
> +
> + if (!find_missing_position(rela->sym, &sp)) {
> + WARN("Unable to find missing symbol: %s",
> + rela->sym->name);
> + return -1;
> + }
> + if (!convert_rela(sec, rela, &sp, klp_elf)) {
> + WARN("Unable to convert relocation: %s",
> + rela->sym->name);
> + return -1;
> + }
> + }
> + }
AFAIK the *_safe list traversals, they cache the ->next value at the
beginning of each iteration, so that one could blow the current element
in position away. The cached ->next value is then assigned when moving
to the next iteration.
But notice how convert_rela() looks through the entire list of
relocations, moving each rela with a matching symbol?
Consider a slight tweak to samples/livepatch-annotated.c:
static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
{
+ if (saved_command_line)
+ saved_command_line[0] = '\0';
+
seq_printf(m, "%s livepatch=1\n", saved_command_line);
return 0;
}
On my system, this generates relocations like this:
% eu-readelf --relocs samples/livepatch/livepatch-annotated-sample.o
Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x98
contains 9 entries:
Offset TypeValue Addend Name
0x0001 X86_64_PC32 00 -4 __fentry__
0x0008 X86_64_PC32 00 -4
saved_command_line
0x0017 X86_64_PC32 00 -4
saved_command_line
0x001e X86_64_32S 00 +0
.rodata.str1.1
0x0023 X86_64_PC32 00 -4 seq_printf
0x0031 X86_64_PC32 00 -4 __fentry__
0x0038 X86_64_32S 00 +0 .data
0x0051 X86_64_PC32 00 -4 __fentry__
0x003d X86_64_PC32 00 -4
klp_enable_patch
We now have back-to-back rela's with sym->name = "saved_command_line".
When the first is converted, convert_rela() will move both of them to
the klp rela section. The linked list values may be consistent, but the
cached ->next value will be bogus and the in-flight-traversal will run
off the rails.
I think we can work around it with a combination of 1) only moving a
single rela symbol at a time in convert_rela and 2) processing the
second (third, etc.) a little bit more so that they are moved
individually.
I hacked this together and it works against the livepatch-annotate.c
test above so far...
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/scripts/livepatch/klp-c
Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/24/19 3:13 PM, Joao Moreira wrote: Future Work --- I don't see an easy way to support multiple homonym symbols with unique values in the current livepatch module Elf format. The only solutions that come to mind right now include renaming homonym symbols somehow to retain the relocation->symbol relationship when separate object files are combined. Perhaps an intermediate linker step could make annotated symbols unique in some way to achieve this. /thinking out loud I'd set this aside for now and we can return to it later. I think it could be quite rare in practice. I agree, especially since we can detect this corner case and abort the translation. I was thinking about renaming the symbol too. We can extend the symbol naming convention we have now and deal with it in klp_resolve_symbols(), but maybe Josh will come up with something clever and cleaner. I think this could work well, but (sorry if I understood Joe's idea wrongly) not as a linker step. Instead of modifying the linker, I think we could create another tool and plug it into the kbuild pipeline prior to the livepatch module linking. This way, we would parse the .o elf files, check for homonyms and rename them based on a convention that is later understood by klp-convert, as suggested. My knowledge of the build tools is limited, so there was a bunch of hand-waving you couldn't see when I wrote that paragraph :) But yes, that is basically the idea: plugging into the kbuild pipeline to give these some kinda of .o-unique prefix that klp-convert would interpret and strip accordingly. If I am not missing something, this would fix the case where we have homonyms pointing to the same or different positions, without additional user intervention other then adding the SYMPOS annotations. If you consider this to be useful I can start experiencing. It's not the highest priority, but even a prototype of how to insert a script into the pipeline to achieve this would be massively time saving for myself. If renaming looks easy, we could try to work into the initial klp-convert patchset... if not, save it for a follow up enhancement. Thanks, -- Joe
Re: [PATCH v3 3/9] livepatch: Add klp-convert tool
On 4/24/19 1:47 PM, Miroslav Benes wrote:
On Tue, 23 Apr 2019, Joe Lawrence wrote:
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 82c27d219372..126395f1c0cd 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct
rela *r,
if (r1->sym->name == r->sym->name) {
list_del(&r1->list);
list_add(&r1->list, &sec->relas);
+ break;
}
}
return true;
Couldn't we remove the loop all together instead of breaking it?
list_del(&r->list);
list_add(&r->list, &sec->relas);
could be sufficient.
Ah yea, good point.
@@ -549,8 +550,8 @@ static bool is_converted(char *sname)
}
/*
- * Checks if symbol must be converted (conditions):
- * not resolved, not already converted or isn't an exported symbol
+ * Checks if symbol must be or was already converted (conditions):
+ * not resolved or isn't an exported symbol
*/
static bool must_convert(struct symbol *sym)
{
@@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym)
if (strcmp(sym->name, ".TOC.") == 0)
return false;
- return (!(is_converted(sym->name) || is_exported(sym->name)));
+ return (!is_exported(sym->name));
}
/* Checks if a section is a klp rela section */
@@ -640,7 +641,8 @@ int main(int argc, const char **argv)
if (!must_convert(rela->sym))
continue;
- if (!find_missing_position(rela->sym, &sp)) {
+ if (!is_converted(rela->sym->name) &&
+ !find_missing_position(rela->sym, &sp)) {
WARN("Unable to find missing symbol: %s",
rela->sym->name);
return -1;
Looks good.
There were a few additional spots I needed to update (like main's
section walk doesn't need to re-convert klp_rela_sections, and likewise,
convert_rela doesn't need to re-convert_klp_symbol), but I will include
with v4 for a real review.
Thanks,
-- Joe
Re: Livepatch vs LTO
On 4/25/19 11:26 AM, Josh Poimboeuf wrote: Hi all, On IRC, Peter expressed some concern about -flive-patching, specifically that the flag isn't compatible with LTO. The upstream kernel currently doesn't support LTO, but Android is using it with LLVM: https://source.android.com/devices/tech/debug/kcfi And there seems to be progress being made in that direction for upstream. Live patching has at least the following issues with LTO: - For source-based patch generation (klp-convert and friends), the GCC manual says that -flive-patching is incompatible with LTO. Does anybody know if that's a hard incompatibility, or can it be fixed? Also, what about the performance implications of this flag with LTO? Might they become more pronounced? Also I wonder if -fdump-ipa-clones works with LTO? I also wonder about the future of source-based patch generation with LLVM. Will it also have -flive-patching and -fdump-ipa-clones flags? - For binary-based patch generation (kpatch-build), we currently diff objects at a per-compilation-unit level. That would have to be changed to work on vmlinux.o instead. - Objtool would also have to be changed to work on vmlinux.o. It's currently not optimized for large files, and the per-.o whitelisting would need to be fixed. And there may be other issues lurking. Also, thinking about objtool in this context has given me another idea, which might allow us to get rid of the use of -flive-patching and -fdump-ipa-clones altogether (which are both nasty and way too compiler-dependent): Would objtool work around these issues because it would (pending the above changes) operate on post-LTO object files? Since objtool is already reading every function in the kernel, it could create a checksum associated with each function, based on all the instructions (both within the function and any alternatives or other special sections it relies on). The function checksums could be written to a file. Then, when a patch file is applied and the kernel rebuilt, the checksum files could be compared to determine exactly which functions have changed at a binary level. Thoughts? Any reasons why that wouldn't work? This is an interesting option. Keep in mind, like kpatch-build, it would detect changes as a result of source code line number positioning, ie WARN_* or might_sleep macros that kpatch-build currently detects and chooses to ignore. Not a big deal, but warts like this start introducing more instruction decoding into the process. Also, I think a klp-convert type script would still be needed to create livepatch symbols and their corresponding sections and relocations, right? However, we might not need manual symbol annotations to pull this off since presumably the object will have already built/linked. I think. I've only just started looking at klp-convert and asm alternatives, but maybe this would also help determine the alteratives-relocation to klp_object relationship that we will need if we want klp-convert to create klp.arch sections. And, if we wanted to take the idea even further, objtool could have the ability to write the changed functions to a new object file. Voila, we now pretty much have kpatch-build :-) (Though whether this is better than source-based patch generation is certainly an open question.) Porting objtool to new arches is probably easier than kpatch-build at least. -- Joe
