2023-02-14 21:32:43

by Srinivasarao Pathipati

[permalink] [raw]
Subject: [PATCH V1] um: Fix compilation warnings

Use dynamic allocation in sig_handler_common() and in
timer_real_alarm_handler() to fix below warnings and build
failures where CONFIG_WERROR is enabled.

arch/um/os-Linux/signal.c: In function ‘sig_handler_common’:
arch/um/os-Linux/signal.c:51:1: error: the frame size of 2960 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
}
^
arch/um/os-Linux/signal.c: In function ‘timer_real_alarm_handler’:
arch/um/os-Linux/signal.c:95:1: error: the frame size of 2960 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
}

Signed-off-by: Srinivasarao Pathipati <[email protected]>
---
arch/um/os-Linux/signal.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
index 24a403a..9de8826 100644
--- a/arch/um/os-Linux/signal.c
+++ b/arch/um/os-Linux/signal.c
@@ -32,23 +32,25 @@ void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *) = {

static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
{
- struct uml_pt_regs r;
+ struct uml_pt_regs *r;
int save_errno = errno;

- r.is_user = 0;
+ r = malloc(sizeof(struct uml_pt_regs));
+ r->is_user = 0;
if (sig == SIGSEGV) {
/* For segfaults, we want the data from the sigcontext. */
- get_regs_from_mc(&r, mc);
- GET_FAULTINFO_FROM_MC(r.faultinfo, mc);
+ get_regs_from_mc(r, mc);
+ GET_FAULTINFO_FROM_MC(r->faultinfo, mc);
}

/* enable signals if sig isn't IRQ signal */
if ((sig != SIGIO) && (sig != SIGWINCH))
unblock_signals_trace();

- (*sig_info[sig])(sig, si, &r);
+ (*sig_info[sig])(sig, si, r);

errno = save_errno;
+ free(r);
}

/*
@@ -99,13 +101,15 @@ void sig_handler(int sig, struct siginfo *si, mcontext_t *mc)

static void timer_real_alarm_handler(mcontext_t *mc)
{
- struct uml_pt_regs regs;
+ struct uml_pt_regs *regs;

+ regs = malloc(sizeof(struct uml_pt_regs));
if (mc != NULL)
- get_regs_from_mc(&regs, mc);
+ get_regs_from_mc(regs, mc);
else
- memset(&regs, 0, sizeof(regs));
- timer_handler(SIGALRM, NULL, &regs);
+ memset(regs, 0, sizeof(struct uml_pt_regs));
+ timer_handler(SIGALRM, NULL, regs);
+ free(regs);
}

void timer_alarm_handler(int sig, struct siginfo *unused_si, mcontext_t *mc)
--
2.7.4



2023-02-14 21:57:09

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings

----- Ursprüngliche Mail -----
> Von: "Srinivasarao Pathipati" <[email protected]>
> static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
> {
> - struct uml_pt_regs r;
> + struct uml_pt_regs *r;
> int save_errno = errno;
>
> - r.is_user = 0;
> + r = malloc(sizeof(struct uml_pt_regs));

I fear this is not correct since malloc() is not async-signal safe.

Thanks,
//richard

2023-02-15 05:36:46

by Srinivasarao Pathipati

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings


On 2/15/2023 3:27 AM, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Srinivasarao Pathipati" <[email protected]>
>> static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
>> {
>> - struct uml_pt_regs r;
>> + struct uml_pt_regs *r;
>> int save_errno = errno;
>>
>> - r.is_user = 0;
>> + r = malloc(sizeof(struct uml_pt_regs));
> I fear this is not correct since malloc() is not async-signal safe.

Thanks Richard for quick response. Could you please suggest alternative
function of malloc() with async-signal safe.

if that is not possible Is there any other way to fix this warning? OR
do we need to live with that warning?

>
> Thanks,
> //richard

2023-02-15 08:07:39

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings

Hi Srinivasarao,

On Wed, Feb 15, 2023 at 6:36 AM Srinivasarao Pathipati
<[email protected]> wrote:
> On 2/15/2023 3:27 AM, Richard Weinberger wrote:
> > ----- Ursprüngliche Mail -----
> >> Von: "Srinivasarao Pathipati" <[email protected]>
> >> static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
> >> {
> >> - struct uml_pt_regs r;
> >> + struct uml_pt_regs *r;
> >> int save_errno = errno;
> >>
> >> - r.is_user = 0;
> >> + r = malloc(sizeof(struct uml_pt_regs));
> > I fear this is not correct since malloc() is not async-signal safe.
>
> Thanks Richard for quick response. Could you please suggest alternative
> function of malloc() with async-signal safe.
>
> if that is not possible Is there any other way to fix this warning? OR
> do we need to live with that warning?

Does this limit actually apply to this file, which calls into the host OS?

How come you even see this warning, as we have

CFLAGS_signal.o += -Wframe-larger-than=4096

since commit 517f60206ee5d5f7 ("um: Increase stack frame size threshold
for signal.c") in v5.11?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2023-02-15 08:13:58

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings

On Wed, 2023-02-15 at 09:07 +0100, Geert Uytterhoeven wrote:
> Hi Srinivasarao,
>
> On Wed, Feb 15, 2023 at 6:36 AM Srinivasarao Pathipati
> <[email protected]> wrote:
> > On 2/15/2023 3:27 AM, Richard Weinberger wrote:
> > > ----- Ursprüngliche Mail -----
> > > > Von: "Srinivasarao Pathipati" <[email protected]>
> > > > static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
> > > > {
> > > > - struct uml_pt_regs r;
> > > > + struct uml_pt_regs *r;
> > > > int save_errno = errno;
> > > >
> > > > - r.is_user = 0;
> > > > + r = malloc(sizeof(struct uml_pt_regs));
> > > I fear this is not correct since malloc() is not async-signal safe.
> >
> > Thanks Richard for quick response. Could you please suggest alternative
> > function of malloc() with async-signal safe.
> >
> > if that is not possible Is there any other way to fix this warning? OR
> > do we need to live with that warning?
>
> Does this limit actually apply to this file, which calls into the host OS?

Not really. Also, we know we have a signal stack that's large enough,
since we set it up ourselves:

set_sigstack((void *) STUB_DATA, UM_KERN_PAGE_SIZE);

and it's a full page, so even the OS eating up some of that won't cause
us any trouble. We do have somewhat deep calls into do_IRQ() but those
really shouldn't use much stack space since they can (in non-UM kernels)
be called on top of arbitrary kernel stacks already.

> How come you even see this warning, as we have
>
> CFLAGS_signal.o += -Wframe-larger-than=4096
>
> since commit 517f60206ee5d5f7 ("um: Increase stack frame size threshold
> for signal.c") in v5.11?
>

Good question, I don't see it. However we probably should make that a
_bit_ smaller since we only have a page and still need to call do_IRQ()
and all.

johannes

2023-02-15 08:14:53

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings

On Tue, 2023-02-14 at 22:57 +0100, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Srinivasarao Pathipati" <[email protected]>
> > static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
> > {
> > - struct uml_pt_regs r;
> > + struct uml_pt_regs *r;
> > int save_errno = errno;
> >
> > - r.is_user = 0;
> > + r = malloc(sizeof(struct uml_pt_regs));
>
> I fear this is not correct since malloc() is not async-signal safe.

It would probably also be a non-trivial performance regression for
'interrupt' handling.

We _could_ use a static if this really was a problem, but that'd be just
one more thing to undo for SMP, and see my other mail, it's really not
needed to worry about htis.

johannes

2023-02-15 08:15:11

by Srinivasarao Pathipati

[permalink] [raw]
Subject: Re: [PATCH V1] um: Fix compilation warnings

Hi Greert Uytterhoeven,

On 2/15/2023 1:37 PM, Geert Uytterhoeven wrote:
> Hi Srinivasarao,
>
> On Wed, Feb 15, 2023 at 6:36 AM Srinivasarao Pathipati
> <[email protected]> wrote:
>> On 2/15/2023 3:27 AM, Richard Weinberger wrote:
>>> ----- Ursprüngliche Mail -----
>>>> Von: "Srinivasarao Pathipati" <[email protected]>
>>>> static void sig_handler_common(int sig, struct siginfo *si, mcontext_t *mc)
>>>> {
>>>> - struct uml_pt_regs r;
>>>> + struct uml_pt_regs *r;
>>>> int save_errno = errno;
>>>>
>>>> - r.is_user = 0;
>>>> + r = malloc(sizeof(struct uml_pt_regs));
>>> I fear this is not correct since malloc() is not async-signal safe.
>> Thanks Richard for quick response. Could you please suggest alternative
>> function of malloc() with async-signal safe.
>>
>> if that is not possible Is there any other way to fix this warning? OR
>> do we need to live with that warning?
> Does this limit actually apply to this file, which calls into the host OS?
>
> How come you even see this warning, as we have
>
> CFLAGS_signal.o += -Wframe-larger-than=4096
>
> since commit 517f60206ee5d5f7 ("um: Increase stack frame size threshold
> for signal.c") in v5.11?
>
> Gr{oetje,eeting}s,
>
> Geert

We were testing this on 5.10 kernel.

We will back port this change.

Thanks