skb_frag_address_safe() needs a check that the
skb_frag_page exists check similar to skb_frag_address().

Cc: ap420073@gmail.com

Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 include/linux/skbuff.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 5520524c93bf..9d551845e517 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3665,7 +3665,12 @@ static inline void *skb_frag_address(const skb_frag_t *frag)
  */
 static inline void *skb_frag_address_safe(const skb_frag_t *frag)
 {
-	void *ptr = page_address(skb_frag_page(frag));
+	void *ptr;
+
+	if (!skb_frag_page(frag))
+		return NULL;
+
+	ptr = page_address(skb_frag_page(frag));
 	if (unlikely(!ptr))
 		return NULL;
 

base-commit: 7b4ac12cc929e281cf7edc22203e0533790ebc2b
-- 
2.50.0.rc2.696.g1fc2a0284f-goog

On Tue, 17 Jun 2025 21:09:50 +0000 Mina Almasry wrote:
> +	void *ptr;
> +
> +	if (!skb_frag_page(frag))
> +		return NULL;
> +
> +	ptr = page_address(skb_frag_page(frag));

Sorry, noticed mid-push that we're calling skb_frag_page() twice.
Let's save the return value and pass it to page_address?
-- 
pw-bot: cr

On Tue, Jun 17, 2025 at 2:09 PM Mina Almasry <almasrymina@google.com> wrote:
>
> skb_frag_address_safe() needs a check that the
> skb_frag_page exists check similar to skb_frag_address().
>
> Cc: ap420073@gmail.com
>

Sorry, I realized right after hitting send, I'm missing:

Fixes: 9f6b619edf2e ("net: support non paged skb frags")

I can respin after the 24hr cooldown.

-- 
Thanks,
Mina

On 06/17, Mina Almasry wrote:
> On Tue, Jun 17, 2025 at 2:09 PM Mina Almasry <almasrymina@google.com> wrote:
> >
> > skb_frag_address_safe() needs a check that the
> > skb_frag_page exists check similar to skb_frag_address().
> >
> > Cc: ap420073@gmail.com
> >
> 
> Sorry, I realized right after hitting send, I'm missing:
> 
> Fixes: 9f6b619edf2e ("net: support non paged skb frags")
> 
> I can respin after the 24hr cooldown.

The function is used in five drivers, none of which support devmem tx,
does not look like there is a reason to route it via net.

The change it self looks good, but not really sure it's needed.
skb_frag_address_safe is used in some pass-data-via-descriptor-ring mode,
I don't see 'modern' drivers (besides bnxt which added this support in 2015)
use it.

On Tue, Jun 17, 2025 at 2:30 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 06/17, Mina Almasry wrote:
> > On Tue, Jun 17, 2025 at 2:09 PM Mina Almasry <almasrymina@google.com> wrote:
> > >
> > > skb_frag_address_safe() needs a check that the
> > > skb_frag_page exists check similar to skb_frag_address().
> > >
> > > Cc: ap420073@gmail.com
> > >
> >
> > Sorry, I realized right after hitting send, I'm missing:
> >
> > Fixes: 9f6b619edf2e ("net: support non paged skb frags")
> >
> > I can respin after the 24hr cooldown.
>
> The function is used in five drivers, none of which support devmem tx,
> does not look like there is a reason to route it via net.
>
> The change it self looks good, but not really sure it's needed.
> skb_frag_address_safe is used in some pass-data-via-descriptor-ring mode,
> I don't see 'modern' drivers (besides bnxt which added this support in 2015)
> use it.

Meh, a judgement call could be made here.  I've generally tried to
make sure skb helpers are (unreadable) netmem compatible without a
thorough analysis of all the callers to make sure they do or will one
day use (unreadable) netmem. Seems better to me to fix this before
some code path that plumbs unreadable memory to the helper is actually
merged and that code starts crashing.

Similarly I put this in net because it's a fix and not a feature. I
can send to net-next if preferred.

On Tue, 17 Jun 2025 14:52:17 -0700 Mina Almasry wrote:
> > > Sorry, I realized right after hitting send, I'm missing:
> > >
> > > Fixes: 9f6b619edf2e ("net: support non paged skb frags")
> > >
> > > I can respin after the 24hr cooldown.  
> >
> > The function is used in five drivers, none of which support devmem tx,
> > does not look like there is a reason to route it via net.
> >
> > The change it self looks good, but not really sure it's needed.
> > skb_frag_address_safe is used in some pass-data-via-descriptor-ring mode,
> > I don't see 'modern' drivers (besides bnxt which added this support in 2015)
> > use it.  
> 
> Meh, a judgement call could be made here.  I've generally tried to
> make sure skb helpers are (unreadable) netmem compatible without a
> thorough analysis of all the callers to make sure they do or will one
> day use (unreadable) netmem. Seems better to me to fix this before
> some code path that plumbs unreadable memory to the helper is actually
> merged and that code starts crashing.

Fair points, tho I prefer the simple heuristic of "can it trigger on
net", otherwise it's really easy to waste time pondering each single
patch. I'll apply to net-next as is. Stanislav, do you want to ack?

On 06/18, Jakub Kicinski wrote:
> On Tue, 17 Jun 2025 14:52:17 -0700 Mina Almasry wrote:
> > > > Sorry, I realized right after hitting send, I'm missing:
> > > >
> > > > Fixes: 9f6b619edf2e ("net: support non paged skb frags")
> > > >
> > > > I can respin after the 24hr cooldown.  
> > >
> > > The function is used in five drivers, none of which support devmem tx,
> > > does not look like there is a reason to route it via net.
> > >
> > > The change it self looks good, but not really sure it's needed.
> > > skb_frag_address_safe is used in some pass-data-via-descriptor-ring mode,
> > > I don't see 'modern' drivers (besides bnxt which added this support in 2015)
> > > use it.  
> > 
> > Meh, a judgement call could be made here.  I've generally tried to
> > make sure skb helpers are (unreadable) netmem compatible without a
> > thorough analysis of all the callers to make sure they do or will one
> > day use (unreadable) netmem. Seems better to me to fix this before
> > some code path that plumbs unreadable memory to the helper is actually
> > merged and that code starts crashing.
> 
> Fair points, tho I prefer the simple heuristic of "can it trigger on
> net", otherwise it's really easy to waste time pondering each single
> patch. I'll apply to net-next as is. Stanislav, do you want to ack?

Sure, SG!

Acked-by: Stanislav Fomichev <sdf@fomichev.me>