| Message ID | 1676410243-10566-1-git-send-email-quic_c_spathi@quicinc.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [V1] um: Fix compilation warnings | expand |
----- Ursprüngliche Mail ----- > Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> > 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
On 2/15/2023 3:27 AM, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- >> Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> >> 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
Hi Srinivasarao, On Wed, Feb 15, 2023 at 6:36 AM Srinivasarao Pathipati <quic_c_spathi@quicinc.com> wrote: > On 2/15/2023 3:27 AM, Richard Weinberger wrote: > > ----- Ursprüngliche Mail ----- > >> Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> > >> 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
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 > <quic_c_spathi@quicinc.com> wrote: > > On 2/15/2023 3:27 AM, Richard Weinberger wrote: > > > ----- Ursprüngliche Mail ----- > > > > Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> > > > > 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
On Tue, 2023-02-14 at 22:57 +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> > > 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
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 > <quic_c_spathi@quicinc.com> wrote: >> On 2/15/2023 3:27 AM, Richard Weinberger wrote: >>> ----- Ursprüngliche Mail ----- >>>> Von: "Srinivasarao Pathipati" <quic_c_spathi@quicinc.com> >>>> 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
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(®s, mc); + get_regs_from_mc(regs, mc); else - memset(®s, 0, sizeof(regs)); - timer_handler(SIGALRM, NULL, ®s); + 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)
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 <quic_c_spathi@quicinc.com> --- arch/um/os-Linux/signal.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)