| Message ID | 20250321081313.37112-4-m-malladi@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bug fixes from XDP and perout series | expand |
On Fri, 21 Mar 2025 13:43:13 +0530 Meghana Malladi wrote: > Whenever there is a perout request from the user application, > kernel receives req structure containing the configuration info > for that req. This doesn't really explain the condition under which the bug triggers. Presumably when user request comes in req is never NULL? > Add NULL pointer handling for perout request if > that req struct points to NULL. > > Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal") > Signed-off-by: Meghana Malladi <m-malladi@ti.com> > Reviewed-by: Simon Horman <horms@kernel.org> > --- > > Changes from v1(v2-v1): > - Collected RB tag from Simon Horman <horms@kernel.org> > > drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c > index b4a34c57b7b4..aeebdc4c121e 100644 > --- a/drivers/net/ethernet/ti/icssg/icss_iep.c > +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c > @@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep, > { > int ret = 0; > > + /* Return error if the req is NULL */ code is trivial here, explain the 'why' not the 'what' Why is this called with NULL? > + if (!req) > + return -EINVAL;
On 3/25/2025 11:18 PM, Jakub Kicinski wrote: > On Fri, 21 Mar 2025 13:43:13 +0530 Meghana Malladi wrote: >> Whenever there is a perout request from the user application, >> kernel receives req structure containing the configuration info >> for that req. > > This doesn't really explain the condition under which the bug triggers. > Presumably when user request comes in req is never NULL? > You are right, I have looked into what would trigger this bug but seems like user request can never be NULL, but the contents inside the req can be invalid, but that is already being handled by the kernel. So this bug fix makes no sense and I will be dropping this patch for v3. Thanks. >> Add NULL pointer handling for perout request if >> that req struct points to NULL. >> >> Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal") >> Signed-off-by: Meghana Malladi <m-malladi@ti.com> >> Reviewed-by: Simon Horman <horms@kernel.org> >> --- >> >> Changes from v1(v2-v1): >> - Collected RB tag from Simon Horman <horms@kernel.org> >> >> drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c >> index b4a34c57b7b4..aeebdc4c121e 100644 >> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c >> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c >> @@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep, >> { >> int ret = 0; >> >> + /* Return error if the req is NULL */ > > code is trivial here, explain the 'why' not the 'what' > Why is this called with NULL? > >> + if (!req) >> + return -EINVAL;
On Fri, Mar 28, 2025 at 11:46:49AM +0530, Malladi, Meghana wrote: > > > On 3/25/2025 11:18 PM, Jakub Kicinski wrote: > > On Fri, 21 Mar 2025 13:43:13 +0530 Meghana Malladi wrote: > > > Whenever there is a perout request from the user application, > > > kernel receives req structure containing the configuration info > > > for that req. > > > > This doesn't really explain the condition under which the bug triggers. > > Presumably when user request comes in req is never NULL? > > > > You are right, I have looked into what would trigger this bug but seems like > user request can never be NULL, but the contents inside the req can be > invalid, but that is already being handled by the kernel. So this bug fix > makes no sense and I will be dropping this patch for v3. Thanks. > I don't remember bug reports for more than a few hours so I had to dig this up on lore: https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/ This is definitely still a real bug on today's linux-next but yes, the fix is bad. drivers/net/ethernet/ti/icssg/icss_iep.c 814 int icss_iep_exit(struct icss_iep *iep) 815 { 816 if (iep->ptp_clock) { 817 ptp_clock_unregister(iep->ptp_clock); 818 iep->ptp_clock = NULL; 819 } 820 icss_iep_disable(iep); 821 822 if (iep->pps_enabled) 823 icss_iep_pps_enable(iep, false); 824 else if (iep->perout_enabled) 825 icss_iep_perout_enable(iep, NULL, false); ^^^^ A better fix probably to delete this function call instead of turning it into a no-op. 826 827 return 0; 828 } regards, dan carpenter
On 3/28/2025 1:32 PM, Dan Carpenter wrote: > On Fri, Mar 28, 2025 at 11: 46: 49AM +0530, Malladi, Meghana wrote: > > > > On 3/25/2025 11: 18 PM, Jakub Kicinski wrote: > > On Fri, 21 Mar 2025 > 13: 43: 13 +0530 Meghana Malladi wrote: > > > Whenever there is a perout > request > ZjQcmQRYFpfptBannerStart > This message was sent from outside of Texas Instruments. > Do not click links or open attachments unless you recognize the source > of this email and know the content is safe. > Report Suspicious > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! > uldqfVdF3CcxCkXE1pKJH1UXrlua- > GvI0DSoHvw1l5Yyu6xCbgDWX8PlhMSuV5lCv48fT7ChvXbQ105c6PHChn2lWCWZMsMcoHjvVwM$> > ZjQcmQRYFpfptBannerEnd > > On Fri, Mar 28, 2025 at 11:46:49AM +0530, Malladi, Meghana wrote: >> >> >> On 3/25/2025 11:18 PM, Jakub Kicinski wrote: >> > On Fri, 21 Mar 2025 13:43:13 +0530 Meghana Malladi wrote: >> > > Whenever there is a perout request from the user application, >> > > kernel receives req structure containing the configuration info >> > > for that req. >> > >> > This doesn't really explain the condition under which the bug triggers. >> > Presumably when user request comes in req is never NULL? >> > >> >> You are right, I have looked into what would trigger this bug but seems like >> user request can never be NULL, but the contents inside the req can be >> invalid, but that is already being handled by the kernel. So this bug fix >> makes no sense and I will be dropping this patch for v3. Thanks. >> > > I don't remember bug reports for more than a few hours so I had to dig > this up on lore: > > https://urldefense.com/v3/__https://lore.kernel.org/ > all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK! > XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$ <https://urldefense.com/v3/__https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK!XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$> > > This is definitely still a real bug on today's linux-next but yes, the > fix is bad. > > drivers/net/ethernet/ti/icssg/icss_iep.c > 814 int icss_iep_exit(struct icss_iep *iep) > 815 { > 816 if (iep->ptp_clock) { > 817 ptp_clock_unregister(iep->ptp_clock); > 818 iep->ptp_clock = NULL; > 819 } > 820 icss_iep_disable(iep); > 821 > 822 if (iep->pps_enabled) > 823 icss_iep_pps_enable(iep, false); > 824 else if (iep->perout_enabled) > 825 icss_iep_perout_enable(iep, NULL, false); > ^^^^ > A better fix probably to delete this function call instead of > turning it into a no-op. Yes agreed, current bug fix is bad. Will have a fix similar to this and post it soon. Thanks. > > 826 > 827 return 0; > 828 } > > regards, > dan carpenter >
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c index b4a34c57b7b4..aeebdc4c121e 100644 --- a/drivers/net/ethernet/ti/icssg/icss_iep.c +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c @@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep, { int ret = 0; + /* Return error if the req is NULL */ + if (!req) + return -EINVAL; + /* Reject requests with unsupported flags */ if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE | PTP_PEROUT_PHASE))
Whenever there is a perout request from the user application, kernel receives req structure containing the configuration info for that req. Add NULL pointer handling for perout request if that req struct points to NULL. Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal") Signed-off-by: Meghana Malladi <m-malladi@ti.com> Reviewed-by: Simon Horman <horms@kernel.org> --- Changes from v1(v2-v1): - Collected RB tag from Simon Horman <horms@kernel.org> drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++ 1 file changed, 4 insertions(+)