| Message ID | 20230725064031.4472-1-ruc_gongyuanjun@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/1] net: ipv4: fix return value check in esp_remove_trailer() | expand |
On Tue, 2023-07-25 at 14:40 +0800, Yuanjun Gong wrote: > return an error number if an unexpected result is returned by > pskb_tirm() in esp_remove_trailer(). > > Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> > --- > net/ipv4/esp4.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c > index ba06ed42e428..b435e3fe4dc6 100644 > --- a/net/ipv4/esp4.c > +++ b/net/ipv4/esp4.c > @@ -732,7 +732,9 @@ static inline int esp_remove_trailer(struct sk_buff *skb) > skb->csum = csum_block_sub(skb->csum, csumdiff, > skb->len - trimlen); > } > - pskb_trim(skb, skb->len - trimlen); > + ret = pskb_trim(skb, skb->len - trimlen); > + if (ret) > + goto out; > > ret = nexthdr[1]; > In what case would you encounter this error? From what I can tell it looks like there are checks in the callers, specifically the call to pskb_may_pull() at the start of esp_input() that will go through and automatically eliminate all the potential reasons for this to fail. So I am not sure what the point is in adding exception handling for an exception that is already handled.
On Tue, Jul 25, 2023 at 10:44:50AM -0700, Alexander H Duyck wrote: > On Tue, 2023-07-25 at 14:40 +0800, Yuanjun Gong wrote: > > return an error number if an unexpected result is returned by > > pskb_tirm() in esp_remove_trailer(). > > > > Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> > > --- > > net/ipv4/esp4.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c > > index ba06ed42e428..b435e3fe4dc6 100644 > > --- a/net/ipv4/esp4.c > > +++ b/net/ipv4/esp4.c > > @@ -732,7 +732,9 @@ static inline int esp_remove_trailer(struct sk_buff *skb) > > skb->csum = csum_block_sub(skb->csum, csumdiff, > > skb->len - trimlen); > > } > > - pskb_trim(skb, skb->len - trimlen); > > + ret = pskb_trim(skb, skb->len - trimlen); > > + if (ret) > > + goto out; > > > > ret = nexthdr[1]; > > > > In what case would you encounter this error? From what I can tell it > looks like there are checks in the callers, specifically the call to > pskb_may_pull() at the start of esp_input() that will go through and > automatically eliminate all the potential reasons for this to fail. So > I am not sure what the point is in adding exception handling for an > exception that is already handled. Good point. pskb_trim should never fail at this point because we've already made the packet completely writeable. Perhaps we could add a comment? Thanks,
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c index ba06ed42e428..b435e3fe4dc6 100644 --- a/net/ipv4/esp4.c +++ b/net/ipv4/esp4.c @@ -732,7 +732,9 @@ static inline int esp_remove_trailer(struct sk_buff *skb) skb->csum = csum_block_sub(skb->csum, csumdiff, skb->len - trimlen); } - pskb_trim(skb, skb->len - trimlen); + ret = pskb_trim(skb, skb->len - trimlen); + if (ret) + goto out; ret = nexthdr[1];
return an error number if an unexpected result is returned by pskb_tirm() in esp_remove_trailer(). Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com> --- net/ipv4/esp4.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)