| Message ID | 20220602112138.8200-1-pjlafren@mtu.edu (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ax25: use GFP_KERNEL over GFP_ATOMIC where possible | expand |
On Thu, 2 Jun 2022 07:21:38 -0400 Peter Lafreniere wrote: > There are a few functions that can sleep that use GFP_ATOMIC. > Here we change allocations to use the more reliable GFP_KERNEL > flag. > > ax25_dev_device_up() is only called during device setup, which is > done in user context. In addition, ax25_dev_device_up() > unconditionally calls ax25_register_dev_sysctl(), which already > allocates with GFP_KERNEL. > > ax25_rt_add() is a static function that is only called from > ax25_rt_ioctl(), which must run in user context already due to > copy_from_user() usage. > > Since it is allowed to sleep in both of these functions, here we > change the functions to use GFP_KERNEL to reduce unnecessary > out-of-memory errors. > > Signed-off-by: Peter Lafreniere <pjlafren@mtu.edu> For merging into the Linux networking trees you'll have to repost next week, this seems like an optimization and we're currently in the merge window period where we only accept fixes.
On Thu, Jun 02, 2022 at 07:21:38AM -0400, Peter Lafreniere wrote: > net/ax25/ax25_dev.c | 4 ++-- > net/ax25/ax25_route.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c > index d2a244e1c260..b264904980a8 100644 > --- a/net/ax25/ax25_dev.c > +++ b/net/ax25/ax25_dev.c > @@ -52,7 +52,7 @@ void ax25_dev_device_up(struct net_device *dev) > { > ax25_dev *ax25_dev; > > - if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_ATOMIC)) == NULL) { > + if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL)) == NULL) { > printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n"); > return; > } > @@ -60,7 +60,7 @@ void ax25_dev_device_up(struct net_device *dev) > refcount_set(&ax25_dev->refcount, 1); > dev->ax25_ptr = ax25_dev; > ax25_dev->dev = dev; > - dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC); > + dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_KERNEL); > ax25_dev->forward = NULL; > > ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE; These two are fine. > diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c > index b7c4d656a94b..c77b848ccfc7 100644 > --- a/net/ax25/ax25_route.c > +++ b/net/ax25/ax25_route.c > @@ -91,7 +91,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route) > kfree(ax25_rt->digipeat); > ax25_rt->digipeat = NULL; > if (route->digi_count != 0) { > - if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) { > + if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_KERNEL)) == NULL) { > write_unlock_bh(&ax25_route_lock); This write lock means it has to be GFP_ATOMIC. > ax25_dev_put(ax25_dev); > return -ENOMEM; > @@ -110,7 +110,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route) > ax25_rt = ax25_rt->next; > } > > - if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_ATOMIC)) == NULL) { > + if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_KERNEL)) == NULL) { > write_unlock_bh(&ax25_route_lock); This change is buggy as well. > ax25_dev_put(ax25_dev); > return -ENOMEM; regards, dan carpenter
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c index d2a244e1c260..b264904980a8 100644 --- a/net/ax25/ax25_dev.c +++ b/net/ax25/ax25_dev.c @@ -52,7 +52,7 @@ void ax25_dev_device_up(struct net_device *dev) { ax25_dev *ax25_dev; - if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_ATOMIC)) == NULL) { + if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL)) == NULL) { printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n"); return; } @@ -60,7 +60,7 @@ void ax25_dev_device_up(struct net_device *dev) refcount_set(&ax25_dev->refcount, 1); dev->ax25_ptr = ax25_dev; ax25_dev->dev = dev; - dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC); + dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_KERNEL); ax25_dev->forward = NULL; ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE; diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c index b7c4d656a94b..c77b848ccfc7 100644 --- a/net/ax25/ax25_route.c +++ b/net/ax25/ax25_route.c @@ -91,7 +91,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route) kfree(ax25_rt->digipeat); ax25_rt->digipeat = NULL; if (route->digi_count != 0) { - if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) { + if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_KERNEL)) == NULL) { write_unlock_bh(&ax25_route_lock); ax25_dev_put(ax25_dev); return -ENOMEM; @@ -110,7 +110,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route) ax25_rt = ax25_rt->next; } - if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_ATOMIC)) == NULL) { + if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_KERNEL)) == NULL) { write_unlock_bh(&ax25_route_lock); ax25_dev_put(ax25_dev); return -ENOMEM;
There are a few functions that can sleep that use GFP_ATOMIC. Here we change allocations to use the more reliable GFP_KERNEL flag. ax25_dev_device_up() is only called during device setup, which is done in user context. In addition, ax25_dev_device_up() unconditionally calls ax25_register_dev_sysctl(), which already allocates with GFP_KERNEL. ax25_rt_add() is a static function that is only called from ax25_rt_ioctl(), which must run in user context already due to copy_from_user() usage. Since it is allowed to sleep in both of these functions, here we change the functions to use GFP_KERNEL to reduce unnecessary out-of-memory errors. Signed-off-by: Peter Lafreniere <pjlafren@mtu.edu> --- net/ax25/ax25_dev.c | 4 ++-- net/ax25/ax25_route.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)