Re: [PATCH v3 11/11] sysctl: treewide: constify the ctl_table argument of handlers

2024-04-29 Thread Heiko Carstens
On Tue, Apr 23, 2024 at 09:54:46AM +0200, Thomas Weißschuh wrote:
> Adapt the proc_hander function signature to make it clear that handlers
> are not supposed to modify their ctl_table argument.
> 
> This is a prerequisite to moving the static ctl_table structs into
> rodata.
> By migrating all handlers at once a lengthy transition can be avoided.
> 
> The patch was mostly generated by coccinelle with the following script:
> 
> @@
> identifier func, ctl, write, buffer, lenp, ppos;
> @@
> 
> int func(
> - struct ctl_table *ctl,
> + const struct ctl_table *ctl,
>   int write, void *buffer, size_t *lenp, loff_t *ppos)
> { ... }
> 
> In addition to the scripted changes some other changes are done:
> 
> * the typedef proc_handler is adapted
> 
> * the prototypes of non-static handler are adapted
> 
> * kernel/seccomp.c:{read,write}_actions_logged() and
>   kernel/watchdog.c:proc_watchdog_common() are adapted as they need to
>   adapted together with the handlers for type-consistency reasons
> 
> Signed-off-by: Thomas Weißschuh 

...

>  arch/s390/appldata/appldata_base.c| 10 ++---
>  arch/s390/kernel/debug.c  |  2 +-
>  arch/s390/kernel/topology.c   |  2 +-
>  arch/s390/mm/cmm.c|  6 +--

Acked-by: Heiko Carstens  # s390

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v8 4/4] kexec, KEYS, s390: Make use of built-in and secondary keyring for signature verification

2022-05-18 Thread Heiko Carstens
On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> From: Michal Suchanek 
> 
> commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> adds support for KEXEC_SIG verification with keys from platform keyring
> but the built-in keys and secondary keyring are not used.
> 
> Add support for the built-in keys and secondary keyring as x86 does.
> 
> Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> Cc: sta...@vger.kernel.org
> Cc: Philipp Rudo 
> Cc: kexec@lists.infradead.org
> Cc: keyri...@vger.kernel.org
> Cc: linux-security-mod...@vger.kernel.org
> Signed-off-by: Michal Suchanek 
> Reviewed-by: "Lee, Chun-Yi" 
> Acked-by: Baoquan He 
> Signed-off-by: Coiby Xu 
> ---
>  arch/s390/kernel/machine_kexec_file.c | 18 +-
>  1 file changed, 13 insertions(+), 5 deletions(-)

As far as I can tell this doesn't have any dependency to the other
patches in this series, so should I pick this up for the s390 tree, or
how will this go upstream?

In any case:
Acked-by: Heiko Carstens 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH 22/30] panic: Introduce the panic post-reboot notifier list

2022-05-12 Thread Heiko Carstens
On Mon, May 09, 2022 at 11:16:10AM -0300, Guilherme G. Piccoli wrote:
> On 27/04/2022 19:49, Guilherme G. Piccoli wrote:
> > Currently we have 3 notifier lists in the panic path, which will
> > be wired in a way to allow the notifier callbacks to run in
> > different moments at panic time, in a subsequent patch.
> > 
> > But there is also an odd set of architecture calls hardcoded in
> > the end of panic path, after the restart machinery. They're
> > responsible for late time tunings / events, like enabling a stop
> > button (Sparc) or effectively stopping the machine (s390).
> > 
> > This patch introduces yet another notifier list to offer the
> > architectures a way to add callbacks in such late moment on
> > panic path without the need of ifdefs / hardcoded approaches.
> > 
> > Cc: Alexander Gordeev 
> > Cc: Christian Borntraeger 
> > Cc: "David S. Miller" 
> > Cc: Heiko Carstens 
> > Cc: Sven Schnelle 
> > Cc: Vasily Gorbik 
> > Signed-off-by: Guilherme G. Piccoli 
> 
> Hey S390/SPARC folks, sorry for the ping!
> 
> Any reviews on this V1 would be greatly appreciated, I'm working on V2
> and seeking feedback in the non-reviewed patches.

Sorry, missed that this is quite s390 specific. So, yes, this looks
good to me and nice to see that one of the remaining CONFIG_S390 in
common code will be removed!

For the s390 bits:
Acked-by: Heiko Carstens 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH 13/30] s390/consoles: Improve panic notifiers reliability

2022-04-30 Thread Heiko Carstens
On Wed, Apr 27, 2022 at 07:49:07PM -0300, Guilherme G. Piccoli wrote:
> Currently many console drivers for s390 rely on panic/reboot notifiers
> to invoke callbacks on these events. The panic() function disables local
> IRQs, secondary CPUs and preemption, so callbacks invoked on panic are
> effectively running in atomic context.
> 
> Happens that most of these console callbacks from s390 doesn't take the
> proper care with regards to atomic context, like taking spinlocks that
> might be taken in other function/CPU and hence will cause a lockup
> situation.
> 
> The goal for this patch is to improve the notifiers reliability, acting
> on 4 console drivers, as detailed below:
> 
> (1) con3215: changed a regular spinlock to the trylock alternative.
> 
> (2) con3270: also changed a regular spinlock to its trylock counterpart,
> but here we also have another problem: raw3270_activate_view() takes a
> different spinlock. So, we worked a helper to validate if this other lock
> is safe to acquire, and if so, raw3270_activate_view() should be safe.
> 
> Notice though that there is a functional change here: it's now possible
> to continue the notifier code [reaching con3270_wait_write() and
> con3270_rebuild_update()] without executing raw3270_activate_view().
> 
> (3) sclp: a global lock is used heavily in the functions called from
> the notifier, so we added a check here - if the lock is taken already,
> we just bail-out, preventing the lockup.
> 
> (4) sclp_vt220: same as (3), a lock validation was added to prevent the
> potential lockup problem.
> 
> Besides (1)-(4), we also removed useless void functions, adding the
> code called from the notifier inside its own body, and changed the
> priority of such notifiers to execute late, since they are "heavyweight"
> for the panic environment, so we aim to reduce risks here.
> Changed return values to NOTIFY_DONE as well, the standard one.
> 
> Cc: Alexander Gordeev 
> Cc: Christian Borntraeger 
> Cc: Heiko Carstens 
> Cc: Sven Schnelle 
> Cc: Vasily Gorbik 
> Signed-off-by: Guilherme G. Piccoli 
> ---
> 
> As a design choice, the option used here to verify a given spinlock is taken
> was the function "spin_is_locked()" - but we noticed that it is not often 
> used.
> An alternative would to take the lock with a spin_trylock() and if it 
> succeeds,
> just release the spinlock and continue the code. But that seemed weird...
> 
> Also, we'd like to ask a good validation of case (2) potential functionality
> change from the s390 console experts - far from expert here, and in our naive
> code observation, that seems fine, but that analysis might be missing some
> corner case.
> 
> Thanks in advance!
> 
>  drivers/s390/char/con3215.c| 36 +++--
>  drivers/s390/char/con3270.c| 34 +++
>  drivers/s390/char/raw3270.c| 18 +++
>  drivers/s390/char/raw3270.h|  1 +
>  drivers/s390/char/sclp_con.c   | 28 +--
>  drivers/s390/char/sclp_vt220.c | 42 +++---
>  6 files changed, 96 insertions(+), 63 deletions(-)

Code looks good, and everything still seems to work. I applied this
internally for the time being, and if it passes testing, I'll schedule
it for the next merge window.

Thanks!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v5 RESEND 0/3] Convert vmcore to use an iov_iter

2022-04-11 Thread Heiko Carstens
On Fri, Apr 08, 2022 at 05:24:28PM +0800, Baoquan He wrote:
> Add Heiko to CC.
> 
> On 04/08/22 at 05:06pm, Baoquan He wrote:
> > Copy the description of v3 cover letter from Willy:
> > ===
> > For some reason several people have been sending bad patches to fix
> > compiler warnings in vmcore recently.  Here's how it should be done.
> > Compile-tested only on x86.  As noted in the first patch, s390 should
> > take this conversion a bit further, but I'm not inclined to do that
> > work myself.
> 
> Forgot adding Heiko to CC again.
> 
> Hi Heiko,
> 
> Andrew worried you may miss the note, "As noted in the first patch,
> s390 should take this conversion a bit further, but I'm not inclined
> to do that work myself." written in cover letter from willy.
> 
> I told him you had already known this in v1 discussion. So add you in CC
> list as Andrew required. Adding words to explain, just in case confusion.

Thanks for letting me know again. I'm still aware of this, but would
appreciate if I could be added to cc in the first patch of this
series, so I get notified when Andrew sends this Linus.

Thanks a lot!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH 1/1] s390: handle R_390_PLT32DBL reloc entries in machine_apply_elf_rel()

2022-01-10 Thread Heiko Carstens
On Mon, Jan 10, 2022 at 04:23:05PM +0100, Alexander Egorenkov wrote:
> Heiko Carstens  writes:
> > Given that Alexander is currently not available, I will resend his
> > patch with an updated commit message.
> 
> Many thanks for the review and taking care of the review finding.

But it looks like this still hasn't been applied to the kexec-tools
git tree, unless I'm mistaken(?)

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH 1/3] vmcore: Convert copy_oldmem_page() to take an iov_iter

2021-12-16 Thread Heiko Carstens
On Mon, Dec 13, 2021 at 08:57:25AM +0100, Christoph Hellwig wrote:
> On Mon, Dec 13, 2021 at 12:06:34AM +, Matthew Wilcox (Oracle) wrote:
> > Instead of passing in a 'buf' and 'userbuf' argument, pass in an iov_iter.
> > s390 needs more work to pass the iov_iter down further, or refactor,
> > but I'd be more comfortable if someone who can test on s390 did that work.
> > 
> > It's more convenient to convert the whole of read_from_oldmem() to
> > take an iov_iter at the same time, so rename it to read_from_oldmem_iter()
> > and add a temporary read_from_oldmem() wrapper that creates an iov_iter.
> 
> This looks pretty reasonable.  s390 could use some love from people that
> know the code, and yes, the kerneldoc comments should go away.

Sure, we will take care of this. Added to ToDo list.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


[PATCHv2 1/1] s390: handle R_390_PLT32DBL reloc entries in machine_apply_elf_rel()

2021-12-15 Thread Heiko Carstens
From: Alexander Egorenkov 

Starting with gcc 11.3, the C compiler will generate PLT-relative function
calls even if they are local and do not require it. Later on during linking,
the linker will replace all PLT-relative calls to local functions with
PC-relative ones. Unfortunately, the purgatory code of kexec/kdump is
not being linked as a regular executable or shared library would have been,
and therefore, all PLT-relative addresses remain in the generated purgatory
object code unresolved. This in turn lets kexec-tools fail with
"Unknown rela relocation: 0x14 0x73c0901c" for such relocation types.

Furthermore, the clang C compiler has always behaved like described above
and this commit should fix the purgatory code built with the latter.

Because the purgatory code is no regular executable or shared library,
contains only calls to local functions and has no PLT, all R_390_PLT32DBL
relocation entries can be resolved just like a R_390_PC32DBL one.

* 
https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_zSeries/x1633.html#AEN1699

Relocation entries of purgatory code generated with gcc 11.3


$ readelf -r purgatory/purgatory.o

Relocation section '.rela.text' at offset 0x6e8 contains 27 entries:
  Offset  Info   Type   Sym. ValueSym. Name + Addend
000c  00030013 R_390_PC32DBL  .data + 2
001a  00100014 R_390_PLT32DBL sha256_starts + 2
0030  00110014 R_390_PLT32DBL sha256_update + 2
0046  00120014 R_390_PLT32DBL sha256_finish + 2
0050  00030013 R_390_PC32DBL  .data + 102
005a  00130014 R_390_PLT32DBL memcmp + 2
...
0118  00160014 R_390_PLT32DBL setup_arch + 2
011e  00030013 R_390_PC32DBL  .data + 2
012c  000f0014 R_390_PLT32DBL 
verify_sha256_digest + 2
0142  00170014 R_390_PLT32DBL
post_verification[...] + 2

Relocation entries of purgatory code generated with gcc 11.2


$ readelf -r purgatory/purgatory.o

Relocation section '.rela.text' at offset 0x6e8 contains 27 entries:
  Offset  Info   Type   Sym. ValueSym. Name + Addend
000e  00030013 R_390_PC32DBL  .data + 2
001c  00100013 R_390_PC32DBL  sha256_starts + 2
0036  00110013 R_390_PC32DBL  sha256_update + 2
0048  00120013 R_390_PC32DBL  sha256_finish + 2
0052  00030013 R_390_PC32DBL  .data + 102
005c  00130013 R_390_PC32DBL  memcmp + 2
...
011a  00160013 R_390_PC32DBL  setup_arch + 2
0120  00030013 R_390_PC32DBL  .data + 122
0130  000f0013 R_390_PC32DBL  
verify_sha256_digest + 2
0146  00170013 R_390_PC32DBL  
post_verification[...] + 2

Corresponding s390 kernel discussion:
* 
https://lore.kernel.org/linux-s390/20211208105801.188140-1-egore...@linux.ibm.com/T/#u

Signed-off-by: Alexander Egorenkov 
Reported-by: Tao Liu 
Suggested-by: Philipp Rudo 
Reviewed-by: Philipp Rudo 
[h...@linux.ibm.com: changed commit message as requested by Philipp Rudo]
Signed-off-by: Heiko Carstens 
---
 kexec/arch/s390/kexec-elf-rel-s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kexec/arch/s390/kexec-elf-rel-s390.c 
b/kexec/arch/s390/kexec-elf-rel-s390.c
index a5e1b7345578..91ba86a9991d 100644
--- a/kexec/arch/s390/kexec-elf-rel-s390.c
+++ b/kexec/arch/s390/kexec-elf-rel-s390.c
@@ -56,6 +56,7 @@ void machine_apply_elf_rel(struct mem_ehdr *UNUSED(ehdr),
case R_390_PC16:/* PC relative 16 bit.  */
case R_390_PC16DBL: /* PC relative 16 bit shifted by 1.  */
case R_390_PC32DBL: /* PC relative 32 bit shifted by 1.  */
+   case R_390_PLT32DBL:/* 32 bit PC rel. PLT shifted by 1.  */
case R_390_PC32:/* PC relative 32 bit.  */
case R_390_PC64:/* PC relative 64 bit.  */
val -= address;
@@ -63,7 +64,7 @@ void machine_apply_elf_rel(struct mem_ehdr *UNUSED(ehdr),
*(unsigned short *) loc = val;
else if (r_type == R_390_PC16DBL)
*(unsigned short *) loc = val >> 1;
-   else if (r_type == R_390_PC32DBL)
+   else if (r_type == R_390_PC32DBL || r_type == R_390_PLT32DBL)
*(unsigned int *) loc = val >> 1;
else if (r_type == R_390_PC32)
 

[PATCHv2 0/1] s390: handle R_390_PLT32DBL reloc entries in machine_apply_elf_rel()

2021-12-15 Thread Heiko Carstens
Version 2 of Alexander Egorenkov's fix with a changed commit message
as pointed out by Philipp Rudo, and requested by Simon Horman.

Please apply.

Alexander Egorenkov (1):
  s390: handle R_390_PLT32DBL reloc entries in machine_apply_elf_rel()

 kexec/arch/s390/kexec-elf-rel-s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.32.0


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH 1/1] s390: handle R_390_PLT32DBL reloc entries in machine_apply_elf_rel()

2021-12-15 Thread Heiko Carstens
On Wed, Dec 15, 2021 at 01:43:00PM +0100, Simon Horman wrote:
> On Mon, Dec 13, 2021 at 11:44:30AM +0100, Philipp Rudo wrote:
> > Hi Alexander,
> > 
> > @Alexander: Thanks for taking care of this.
> > 
> > On Wed,  8 Dec 2021 13:53:55 +0100
> > Alexander Egorenkov  wrote:
> > 
> > > Starting with gcc 11.3, the C compiler will generate PLT-relative function
> > > calls even if they are local and do not require it. Later on during 
> > > linking,
> > > the linker will replace all PLT-relative calls to local functions with
> > > PC-relative ones. Unfortunately, the purgatory code of kexec/kdump is
> > > not being linked as a regular executable or shared library would have 
> > > been,
> > > and therefore, all PLT-relative addresses remain in the generated 
> > > purgatory
> > > object code unresolved. This leads to the situation where the purgatory
> > > code is being executed during kdump with all PLT-relative addresses
> > > unresolved. And this results in endless loops within the purgatory code.
> > 
> > Tiny nit. The last two sentences describe the situation in the kernel.
> > Luckily the kexec-tools do proper error checking and die with
> > 
> > "Unknown rela relocation: 0x14 0x73c0901c"
> > 
> > when they encounter an unknown relocation type.
> > 
> > Anyway, the code is correct
> > 
> > Reviewed-by: Philipp Rudo 
> 
> Thanks.
> 
> Alexander would you care to post a v2 with an updated patch description?

Given that Alexander is currently not available, I will resend his
patch with an updated commit message.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

2021-11-30 Thread Heiko Carstens
On Thu, Nov 25, 2021 at 07:02:38PM +0100, Michal Suchanek wrote:
> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
> verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig | 11 +
>  arch/powerpc/kexec/elf_64.c  | 14 ++
>  arch/s390/kernel/machine_kexec_file.c| 42 ++
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h |  1 +
>  include/linux/verification.h |  4 ++
>  kernel/module-internal.h |  2 -
>  kernel/module.c  | 12 +++--
>  kernel/module_signature.c| 56 +++-
>  kernel/module_signing.c  | 33 +++---
>  security/integrity/ima/ima_modsig.c  | 22 ++
>  11 files changed, 113 insertions(+), 85 deletions(-)

For all patches which touch s390:
Acked-by: Heiko Carstens 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v2 2/2] s390/kexec: fix kmemleak

2021-11-18 Thread Heiko Carstens
On Thu, Nov 18, 2021 at 03:13:27PM +0800, Baoquan He wrote:
> On 11/18/21 at 05:46am, kernel test robot wrote:
> >arch/s390/kernel/machine_kexec_file.c: In function 
> > 'arch_kimage_file_post_load_cleanup':
> > >> arch/s390/kernel/machine_kexec_file.c:332:9: error: implicit declaration 
> > >> of function 'kvfree'; did you mean 'vfree'? 
> > >> [-Werror=implicit-function-declaration]
> >  332 | kvfree(image->arch.ipl_buf);
> >  | ^~
> >  | vfree
> 
> OK, kvfree is not wrong, seems vfree is more appropriate since it's
> clear the ipl_buf is allocated with zvalloc() in ipl_report_finish().
> 
> Hi Heiko,
> 
> Could you help modify the code in your tree or append below patch to
> mute the lkp complaint? Sorry for the inconvenience.
...
>  arch/s390/kernel/machine_kexec_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kernel/machine_kexec_file.c 
> b/arch/s390/kernel/machine_kexec_file.c
> index 7f51837e9bc2..351a7ff69a43 100644
> --- a/arch/s390/kernel/machine_kexec_file.c
> +++ b/arch/s390/kernel/machine_kexec_file.c
> @@ -329,7 +329,7 @@ int arch_kexec_apply_relocations_add(struct 
> purgatory_info *pi,
>  
>  int arch_kimage_file_post_load_cleanup(struct kimage *image)
>  {
> - kvfree(image->arch.ipl_buf);
> + vfree(image->arch.ipl_buf);

The problem reported above indicates that slab.h was not
included. With your patch, while it fixes the problem for this
particular configuration, this requires vmalloc.h to be included.

I'll merge your patch and add the missing include as well.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v2 1/2] s390/kexec: check the return value of ipl_report_finish

2021-11-16 Thread Heiko Carstens
On Tue, Nov 16, 2021 at 11:25:56AM +0800, Baoquan He wrote:
> In function ipl_report_finish(), it could fail by memory allocation
> failure, so check the return value to handle the case.
> 
> Signed-off-by: Baoquan He 
> ---
>  arch/s390/include/asm/ipl.h   | 2 +-
>  arch/s390/kernel/ipl.c| 6 --
>  arch/s390/kernel/machine_kexec_file.c | 5 -
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/ipl.h b/arch/s390/include/asm/ipl.h
> index 3f8ee257f9aa..864ab5d2890c 100644
> --- a/arch/s390/include/asm/ipl.h
> +++ b/arch/s390/include/asm/ipl.h
> @@ -122,7 +122,7 @@ struct ipl_report_certificate {
>  
>  struct kexec_buf;
>  struct ipl_report *ipl_report_init(struct ipl_parameter_block *ipib);
> -void *ipl_report_finish(struct ipl_report *report);
> +int ipl_report_finish(struct ipl_report *report, void **ipl_buf);
>  int ipl_report_free(struct ipl_report *report);
>  int ipl_report_add_component(struct ipl_report *report, struct kexec_buf 
> *kbuf,
>unsigned char flags, unsigned short cert);
> diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
> index e2cc35775b99..a0af0b23148d 100644
> --- a/arch/s390/kernel/ipl.c
> +++ b/arch/s390/kernel/ipl.c
> @@ -2144,7 +2144,7 @@ struct ipl_report *ipl_report_init(struct 
> ipl_parameter_block *ipib)
>   return report;
>  }
>  
> -void *ipl_report_finish(struct ipl_report *report)
> +int ipl_report_finish(struct ipl_report *report, void **ipl_buf)
>  {
>   struct ipl_report_certificate *cert;
>   struct ipl_report_component *comp;
> @@ -2195,7 +2195,9 @@ void *ipl_report_finish(struct ipl_report *report)
>   }
>  
>   BUG_ON(ptr > buf + report->size);
> - return buf;
> + *ipl_buf = buf;
> +
> + return 0;

This does not compile:

  CC  arch/s390/kernel/ipl.o
arch/s390/kernel/ipl.c: In function ‘ipl_report_finish’:
arch/s390/kernel/ipl.c:2159:24: warning: returning ‘void *’ from a function 
with return type ‘int’ makes integer from pointer without a cast 
[-Wint-conversion]
 2159 | return ERR_PTR(-ENOMEM);
  |^~~~

Anyway, before we are going to have more iterations I just applied the
patch below instead before applying your memory leak fix.

From 78e5f268d1be775354ab83c1e039dcfacaa5e258 Mon Sep 17 00:00:00 2001
From: Heiko Carstens 
Date: Tue, 16 Nov 2021 11:06:38 +0100
Subject: s390/kexec: fix return code handling

kexec_file_add_ipl_report ignores that ipl_report_finish may fail and
can return an error pointer instead of a valid pointer.
Fix this and simplify by returning NULL in case of an error and let
the only caller handle this case.

Fixes: 99feaa717e55 ("s390/kexec_file: Create ipl report and pass to next 
kernel")
Signed-off-by: Heiko Carstens 
---
 arch/s390/kernel/ipl.c| 3 ++-
 arch/s390/kernel/machine_kexec_file.c | 8 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
index e2cc35775b99..5ad1dde23dc5 100644
--- a/arch/s390/kernel/ipl.c
+++ b/arch/s390/kernel/ipl.c
@@ -2156,7 +2156,7 @@ void *ipl_report_finish(struct ipl_report *report)
 
buf = vzalloc(report->size);
if (!buf)
-   return ERR_PTR(-ENOMEM);
+   goto out;
ptr = buf;
 
memcpy(ptr, report->ipib, report->ipib->hdr.len);
@@ -2195,6 +2195,7 @@ void *ipl_report_finish(struct ipl_report *report)
}
 
BUG_ON(ptr > buf + report->size);
+out:
return buf;
 }
 
diff --git a/arch/s390/kernel/machine_kexec_file.c 
b/arch/s390/kernel/machine_kexec_file.c
index 528edff085d9..f0200b503f94 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -170,6 +170,7 @@ static int kexec_file_add_ipl_report(struct kimage *image,
struct kexec_buf buf;
unsigned long addr;
void *ptr, *end;
+   int ret;
 
buf.image = image;
 
@@ -199,7 +200,10 @@ static int kexec_file_add_ipl_report(struct kimage *image,
ptr += len;
}
 
+   ret = -ENOMEM;
buf.buffer = ipl_report_finish(data->report);
+   if (!buf.buffer)
+   goto out;
buf.bufsz = data->report->size;
buf.memsz = buf.bufsz;
 
@@ -209,7 +213,9 @@ static int kexec_file_add_ipl_report(struct kimage *image,
data->kernel_buf + offsetof(struct lowcore, ipl_parmblock_ptr);
*lc_ipl_parmblock_ptr = (__u32)buf.mem;
 
-   return kexec_add_buffer(&buf);
+   ret = kexec_add_buffer(&buf);
+out:
+   return ret;
 }
 
 void *kexec_file_add_components(struct kimage *image,
-- 
2.31.1

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v2 RESEND 2/2] s390/kexec: fix memory leak of ipl report buffer

2021-11-16 Thread Heiko Carstens
On Tue, Nov 16, 2021 at 11:31:01AM +0800, Baoquan He wrote:
> unreferenced object 0x38000195000 (size 4096):
>   comm "kexec", pid 8548, jiffies 4294953647 (age 32443.270s)
>   hex dump (first 32 bytes):
> 00 00 00 c8 20 00 00 00 00 00 00 c0 02 80 00 00   ...
> 40 40 40 40 40 40 40 40 00 00 00 00 00 00 00 00  
>   backtrace:
> [<11a2f199>] __vmalloc_node_range+0xc0/0x140
> [<81fa2752>] vzalloc+0x5a/0x70
> [<63a4c92d>] ipl_report_finish+0x2c/0x180
> [<553304da>] kexec_file_add_ipl_report+0xf4/0x150
> [<862d033f>] kexec_file_add_components+0x124/0x160
> [<0d2717bb>] arch_kexec_kernel_image_load+0x62/0x90
> [<2e0373b6>] kimage_file_alloc_init+0x1aa/0x2e0
> [<60f2d14f>] __do_sys_kexec_file_load+0x17c/0x2c0
> [<8c86fe5a>] __s390x_sys_kexec_file_load+0x40/0x50
> [<1fdb9dac>] __do_syscall+0x1bc/0x1f0
> [<3ee4258d>] system_call+0x78/0xa0
> 
> Signed-off-by: Baoquan He 
> Fixes: 99feaa717e55 ("s390/kexec_file: Create ipl report and pass to next 
> kernel")
> ---
> RESEND:
>   Fix the incorrect subject.
> 
>  arch/s390/include/asm/kexec.h | 7 +++
>  arch/s390/kernel/machine_kexec_file.c | 9 +
>  2 files changed, 16 insertions(+)

Applied, thanks!

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH] s390/kexec: fix memory leak of ipl report buffer

2021-11-10 Thread Heiko Carstens
On Fri, Oct 29, 2021 at 06:31:32PM +0200, Philipp Rudo wrote:
> Hi Baoquan,
> 
> On Fri, 29 Oct 2021 17:26:35 +0800
> Baoquan He  wrote:
> 
> > A memory leak is reported by kmemleak scanning:
...
> > The ipl report buffer is allocated via vmalloc, while has no chance to free
> > if the kexe loading is unloaded. This will cause obvious memory leak
> > when kexec/kdump kernel is reloaded, manually, or triggered, e.g by
> > memory hotplug.
> > 
> > Here, add struct kimage_arch to s390 to pass out the ipl report buffer
> > address, and introduce arch dependent function
> > arch_kimage_file_post_load_cleanup() to free it when unloaded.
> > 
> > Signed-off-by: Baoquan He 
> 
> other than a missing
> 
> Fixes: 99feaa717e55 ("s390/kexec_file: Create ipl report and pass to
> next kernel")
> 
> the patch looks good to me.
> 
> Reviewed-by: Philipp Rudo 
...
> > buf.buffer = ipl_report_finish(data->report);
> > buf.bufsz = data->report->size;
> > buf.memsz = buf.bufsz;
> > +   image->arch.ipl_buf = buf.buffer;

This seems (still) to be incorrect: ipl_report_finish() may return
-ENOMEN, but there is no error checking anywhere, as far as I can
tell, which would make this:

> > +int arch_kimage_file_post_load_cleanup(struct kimage *image)
> > +{
> > +   kvfree(image->arch.ipl_buf);
> > +   image->arch.ipl_buf = NULL;
> > +
> > +   return kexec_image_post_load_cleanup_default(image);
> > +}

most likely not do what we want. That is: if this code is reached at
all in such a case. I'll check and might add a patch before this to
fix this also.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v1 2/4] memblock: allow to specify flags with memblock_add_node()

2021-09-28 Thread Heiko Carstens
On Mon, Sep 27, 2021 at 05:05:16PM +0200, David Hildenbrand wrote:
> We want to specify flags when hotplugging memory. Let's prepare to pass
> flags to memblock_add_node() by adjusting all existing users.
> 
> Note that when hotplugging memory the system is already up and running
> and we don't want to add the memory first and apply flags later: it
> should happen within one memblock call.
> 
> Signed-off-by: David Hildenbrand 
> ---
...
>  arch/s390/kernel/setup.c     | 3 ++-

For s390
Acked-by: Heiko Carstens 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: kdump: No udev events for memory hotplug?

2011-10-27 Thread Heiko Carstens
On Wed, Oct 26, 2011 at 03:24:49PM -0400, Vivek Goyal wrote:
> On Wed, Oct 26, 2011 at 06:08:12PM +0200, Michael Holzheu wrote:
> > Hello Vivek and Eric,
> > 
> > I noticed that on my system kernel 3.1 *no* udev events for memory
> > hotplug are generated. Same on my RHEL6.1.
> > 
> > # udevadm monitor
> > # echo offline > /sys/devices/system/memory/memory4/state
> > 
> > -> No event
> > 
> > But we need the udev events in order to do a kdump reload for setting up
> > the ELF loads correctly.
> > 
> > In my /etc/udev/rules.d/98-kexec.rules there are rules for memory
> > hotplug:
> > 
> > SUBSYSTEM=="memory", ACTION=="add", PROGRAM="/etc/init.d/kdump restart"
> > SUBSYSTEM=="memory", ACTION=="remove", PROGRAM="/etc/init.d/kdump
> > restart"
> > 
> > Perhaps/probably I am missing something?
> 
> I don't know. Sounds like a bug. I have never looked into it. CCing Kay,
> if he has any thoughts.
> 
> Are any events generated for memory add?

Looks like uevents are only genereted when memory gets registered and
unregistered, but not when when it gets set online or offline.
To achieve that you would need to add similar code to
store_mem_state()/memory_block_change_state() in drivers/base/memory.c
like we have it already in store_online() in drivers/base/cpu.c

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec